Opened 18 months ago

Closed 10 months ago

Last modified 7 months 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-3.5-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: kzar, sebastian Blocked By:
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Ross Verified working: yes
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 (14)

comment:1 Changed 18 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 18 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 16 months ago by jsonesen

  • Owner set to jsonesen

comment:4 follow-up: Changed 16 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 16 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 16 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 16 months ago by jsonesen

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

comment:8 Changed 16 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 16 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.

Version 0, edited 16 months ago by jsonesen (next)

comment:10 Changed 16 months ago by jsonesen

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

comment:11 Changed 10 months ago by jsonesen

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

comment:12 Changed 10 months 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 10 months ago by jsonesen

  • Review URL(s) modified (diff)

comment:14 Changed 7 months ago by Ross

  • Verified working set

Fixed. Error no longer occurs when using Block element.

ABP 3.4.3.2253
Firefox 65.0.1 / 51 / Windows 10
Firefox Mobile 65.0.1 / Android 7.1.1
Chrome 72.0.3626.109 / 49.0.2623.75 / Windows 10
Opera 58.0.3135.65 / 36.0.2130.80 / Windows 10
Edge 44.17763.1.0 / Windows 10

Note: See TracTickets for help on using tickets.