Opened 9 months ago

Closed 5 weeks ago

Last modified 5 weeks ago

#6490 closed defect (fixed)

"browserAction.setIcon: No tab with id" error after using block element in Chrome

Reported by: Ross Assignee: jsonesen
Priority: P2 Milestone: Adblock-Plus-for-Chrome-Opera-Firefox-next
Module: Platform Keywords:
Cc: kzar, sebastian Blocked By:
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Ross Verified working: no
Review URL(s):

https://gitlab.com/eyeo/adblockplus/adblockpluschrome/merge_requests/12

Description (last modified by kzar)

Environment

ABP 3.0.2.1983
Chrome 49 / 62 / Windows 7

Could not reproduce in Firefox or Opera.

How to reproduce

  1. Navigate to a website (any).
  2. Select [ABP Icon] > [Block element].
  3. Select an element (any) and OK the dialog.

Observed behaviour

The element is hidden as expected however the following error appears in the console (tab id varies):

    extensions::lastError:133 Unchecked runtime.lastError while running browserAction.setIcon: No tab with id: 34.
    reportIfUnchecked @ extensions::lastError:133
    handleResponse @ extensions::sendRequest:78

Expected behaviour

Error to not occur/be handled.

Change History (13)

comment:1 Changed 9 months ago by kzar

  • Description modified (diff)
  • Priority changed from Unknown to P3
  • Ready set

I can reproduce this, seems it's only when the filter generated is a request blocking rather than element hiding one.

comment:2 Changed 9 months ago by sebastian

  • Priority changed from P3 to P2

Reproduced, here, as well. This seems to be a regression since the last release for Chrome (1.13.5). However, if this is the only known regression, and since it doesn't seem to have any visible effects to the user, it seems ok if we fix this with Adblock Plus 3.0.4, rather than blocking the 3.0.3 release.

comment:3 Changed 7 months ago by jsonesen

  • Owner set to jsonesen

comment:4 follow-up: Changed 7 months ago by sebastian

It appears to be a race condition: When any filter is added (or removed) we update the icon for each tab and window (including the "Block element" dialog). By the time borwser.tabs.query({}) is called the "Block element" dialog still exists, but by the time we update the icon (a couple milliseconds later) it has been removed causing this error.

comment:5 in reply to: ↑ 4 Changed 7 months ago by jsonesen

Replying to sebastian:

By the time borwser.tabs.query({}) is called the "Block element" dialog still exists, but by the time we update the icon (a couple milliseconds later) it has been removed causing this error.

In this case should we first check the dialogue exists? or do you think it is better to catch the error and continue?

comment:6 Changed 7 months ago by sebastian

Though less likely, there can be other windows/tabs (besides the "Block element" dialog) getting removed in between detecting filter changes and updating the icon. So I don't think that simply ignoring the "Block element" dialog would be an appropriate solution.

I wonder if we can mitigate the race condition if call browser.borwserAction.set*() imediatelly rather than first checking if the tab exists first.

Otherwise, (while not great) silencing the error seems to be the best option left.

comment:7 Changed 7 months ago by jsonesen

It looks like commenting out the tab.get block stops the error.

comment:8 Changed 7 months ago by sebastian

We still need to handle the scenario of pre-rendered tabs, but perhaps we can just handle browserAction.set*() failing first to not defer the action unnecessarily in the common case.

comment:9 Changed 6 months ago by jsonesen

It seems we already catch errors in setIcon() for Edge here perhaps I can check the error message at the start of the catch and silence the error? Should avoid common case handling too.

EDIT:
That's exception handling whoops, but adding a callback to the setIcon call which reads browser.runtime.lastError works and avoids running the check on all set* but doesn't avoid it for common setIcon case.

Last edited 6 months ago by jsonesen (previous) (diff)

comment:10 Changed 6 months ago by jsonesen

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:11 Changed 5 weeks ago by jsonesen

A commit referencing this issue has landed:
Issue 6940 - Wrap browser.browserAction.set* with promise

comment:12 Changed 5 weeks ago by jsonesen

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:13 Changed 5 weeks ago by jsonesen

  • Review URL(s) modified (diff)
Note: See TracTickets for help on using tickets.