1654842 - Adblock Plus causes tab.discarded == false for all tabs on startup

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P2)

Description

[JulianHofstadter]

Attachment: testDiscarded.xpi

With default preferences, unpinned tabs should start up in discarded state (tab.discarded == true). When Adblock Plus is installed, all tabs including unpinned tabs have tab.discarded == false on startup.

One thing interesting about this is that even though with ABP installed unpinned tabs have tab.discarded == false on startup, clicking on them will cause them to appear to load from a blank tab, as though it they were actually discarded.

The issue manifests in 78.0.2 and latest Nightly.

Steps to reproduce:

Create fresh profile

Open in Nightly (to be able to run attached unsigned addon)

Check "Restore Previous Session" in about:preferences

Open browser console; check "Show Content Messages" from gearwheel

Install attached addon, testDiscarded.xpi

Open several tabs

Restart Nightly

Click the green square "testDiscarded" browserAction icon

Observe a list appears of tabs tab.discarded property in browser console (also dumped to output if browser.dom.window.dump.enabled is true)

Observe that list shows "discarded? true" for all unpinned tabs except the active one.

Install Adblock Plus

Click the green square "testDiscarded" browserAction icon

Observe that list now shows "discarded? false" for all unpinned tabs
(Expected: should still show "discarded? true" for all unpinned tabs)

Restart Nightly

Click the green square "testDiscarded" browserAction icon

Observe that list again shows "discarded? false" for all unpinned tabs
(Expected: should show "discarded? true" for all unpinned tabs)

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[JulianHofstadter]

If I remove content_scripts from manifest.json in an experimental clone of ABP, the issue is still present, seeming to indicate it is not a case of contents scripts forcing tabs to become un-discarded.

Comment 3

[JulianHofstadter]

AFAIK, the only way an extension should be able to change discarded state would be by activating the tab. If there are other ways, please advise. Otherwise it seems an extension should not have the power to change discarded state.

After removing any instance of tabs.update(tabId, {active: true}) from ABP code, the issue is still present. Thus I filed a FF bug thinking it possible there is a hole in the tabs API.

Comment 4

[Alex Cornestean]

Hello,

I’ve managed to reproduce the issue on the latest Nightly (81.0a1/20200729213824), Beta Dev Edition (80.0b1/20200728204253) and Unbranded Release (79.0/20200720193547) under Windows 10 Pro 64-bit and Ubuntu 16.04 LTS.
The Dev Edition and Unbranded versions were used as these permit the installation of unsigned add-ons by changing the xpinstall.signatures.required pref to false, similar to Nightly versions.

Please note that following the provided STR did not output the results of clicking the browserAction icon in the browser console but instead in the webextension console, however, the results were as mentioned in the report.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Needinfo-ing myself to look into this (and if usage of the webNavigation API is behind it)

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

ABP does trigger this issue by calling browser.webNavigation.getAllFrames on all the opened tabs.

In Nightly there is also a message logged to warn about it (not actually marked as a warning but as a regular log from here) with the following stacktrace:

      [bug 1345098] Lazy browser prematurely inserted via 'messageManager' property access:
      getter@chrome://browser/content/tabbrowser.js:2165:39
      getAllFrames@chrome://extensions/content/parent/ext-webNavigation.js:236:51
      recvAPICall/result</<@resource://gre/modules/ExtensionParent.jsm:944:68
      withPendingBrowser@resource://gre/modules/ExtensionParent.jsm:485:26
      recvAPICall/result<@resource://gre/modules/ExtensionParent.jsm:944:24
      callAndLog@resource://gre/modules/ExtensionParent.jsm:906:14
      recvAPICall@resource://gre/modules/ExtensionParent.jsm:943:25

ABP may prevent this from their side by checking explicitly if tab.discarded is true before calling webNavigation.getAllFrames.

On the webNavigation API side, we should actually prevent this issue from being triggered, I just checked what is the behavior on chrome and calling webNavigation.getAllFrame or webNavigation.getFrame does call the callback with null as result of the API call and no chrome.runtime.lastError (which with the promise-based API we provide in Firefox would translate into resolving the API call with null).

Another option would be to reject the API call with an explicit error that says that the tab was discarded, but for chrome compatibility reasons of these shared APIs it may be better to just resolve them to null to match the behavior on Chrome.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1654842 - webNavigation.getAllFrames/getFrame should resolve to null when called on discarded tabs. r?mixedpuppy!

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

As an additional side note: while writing a test case to cover the fix I also noticed that currently the webNavigation.getFrame/getAllFrames API calls are never resolved when called on a discarded tab.

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e1e04cc2f221d2e2ae494892d77add8405280d9

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6e471bef8c0
webNavigation.getAllFrames/getFrame should resolve to null when called on discarded tabs. r=mixedpuppy

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e6e471bef8c0

Comment 12

[Julien Cristau [:jcristau]]

Should we uplift to beta? Is esr78 affected?

Comment 13

[Alex Cornestean]

Hello,

I’ve also reproduced the issue on the latest ESR (78.0esr/20200623021425) under Windows 10 Pro 64-bit and Ubuntu 16.04 LTS.

Marking ESR78 as affected, as well.

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Julien Cristau [:jcristau] from comment #12)

Should we uplift to beta? Is esr78 affected?

The fix is pretty small, and the new behavior is the same already implemented on chrome and so it should be safe to uplift it if we want.

I'm going to create the uplift requests.

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 9167396 [details]
Bug 1654842 - webNavigation.getAllFrames/getFrame should resolve to null when called on discarded tabs. r?mixedpuppy!

Beta/Release Uplift Approval Request

• User impact if declined: For users that have ABP installed ([1]) all the non pinned tabs would be undiscarded as soon as the extension has been startup during the Firefox startup.
  [1] AdBlockPlus is explicitly mentioned because it is likely the most commonly installed extension that we know is triggering this issue, but there may be more that have not been reported but are also triggering the same issue
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Not strictly mandatory given the size and kind of fix applied, but it may be still good to verify it before uplifting it.
  Comment 0 does provide some steps to reproduce the issue and a small test extension to more easily verify that the tabs have not been discarded.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is small and only applied to the two webNavigation.getFrame/getAllFrames API methods, the new behavior is the one Chrome implement and so many extension should already expect the new behavior presented on Firefox.
  The fix is also covered by new test cases.
• String changes made/needed:

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 9167396 [details]
Bug 1654842 - webNavigation.getAllFrames/getFrame should resolve to null when called on discarded tabs. r?mixedpuppy!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Undiscarding all tabs right after Firefox startup may have an impact of the performance (the amount and how much the performance impact can be perceived by the user would likely depend from how many tabs are being restored on startup).
  Also, without the fix any call to webNavigation.getFrame/getAllFrames made while some tabs are discarded would never be resolved, and this may likely have some other impacts.
• User impact if declined: For users that have ABP installed ([1]) all the non pinned tabs would be undiscarded as soon as the extension has been startup during the Firefox startup.
  [1] AdBlockPlus is explicitly mentioned because it is likely the most commonly installed extension that we know is triggering this issue, but there may be more that have not been reported but are also triggering the same issue
• Fix Landed on Version: 81
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is small and only applied to the two webNavigation.getFrame/getAllFrames API methods, the new behavior is the one Chrome implement and so many extension should already expect the new behavior presented on Firefox.
  The fix is also covered by new test cases.
• String or UUID changes made by this patch:

Comment 17

[Alex Cornestean]

Verified the fix using the latest Nightly (81.0a1/20200806215439) under Windows 10 Pro 64-bit and Ubuntu 16.04 LTS.

Following the original STR, after installing Adblock Plus and clicking the browserAction icon, all unpinned tabs are properly shown as "discarded? True", except the active one, confirming the fix.

Waiting for the uplift to Beta and ESR and will also verify them as they land. Keeping the qe-verify+ flag until then.

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 9167396 [details]
Bug 1654842 - webNavigation.getAllFrames/getFrame should resolve to null when called on discarded tabs. r?mixedpuppy!

approved for 80.0b6 and 78.2esr

Comment 19

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/db1facdaa409

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/1694afa75c93

Comment 21

[Alex Cornestean]

Verified the fix using the latest Beta Dev Edition (80.0b6/20200807195315) under Windows 10 Pro 64-bit and Ubuntu 16.04 LTS.

Following the original STR, after installing Adblock Plus and clicking the browserAction icon, all unpinned tabs are properly shown as "discarded? True", except the active one, confirming the fix.

Comment 22

[Alex Cornestean]

Verified the fix using the latest ESR (78.2.0esr/20200817153328) under Windows 10 Pro 64-bit and Ubuntu 16.04 LTS.

Following the original STR, after installing Adblock Plus and clicking the browserAction icon, all unpinned tabs are properly shown as "discarded? True", except the active one, confirming the fix.

Comment 23

[Philipp Kewisch [:Fallen][📱📆]]

I've updated https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/webNavigation/getAllFrames and https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/webNavigation/getFrame to mention that the promise is resolved with null if the tab is discarded.