Closed Bug 1654842 Opened 4 years ago Closed 4 years ago

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

Categories

(WebExtensions :: General, defect, P2)

defect

Tracking

(firefox-esr78 verified, firefox79 wontfix, firefox80 verified, firefox81 verified)

VERIFIED FIXED
81 Branch
Tracking Status
firefox-esr78 --- verified
firefox79 --- wontfix
firefox80 --- verified
firefox81 --- verified

People

(Reporter: julianhofstadter, Assigned: rpl)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

Attached file 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)

Attachment #9165713 - Attachment description: Extension to display tab.discarded state of all tabs → testDiscarded.xpi

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

Component: Untriaged → Tabbed Browser

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.

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.

Component: Tabbed Browser → Untriaged
Product: Firefox → WebExtensions

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.

Status: UNCONFIRMED → NEW
Has STR: --- → yes
Ever confirmed: true

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

Flags: needinfo?(lgreco)
Severity: -- → S3
Flags: needinfo?(lgreco)
Flags: needinfo?(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.

Flags: needinfo?(lgreco)
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Priority: -- → P2

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.

Component: Untriaged → General
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/e6e471bef8c0
webNavigation.getAllFrames/getFrame should resolve to null when called on discarded tabs. r=mixedpuppy
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch

Should we uplift to beta? Is esr78 affected?

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.

(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.

Flags: needinfo?(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:
Attachment #9167396 - Flags: approval-mozilla-beta?
Flags: qe-verify+

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:
Attachment #9167396 - Flags: approval-mozilla-esr78?
QA Whiteboard: [qa-triaged]

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.

Status: RESOLVED → VERIFIED

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

Attachment #9167396 - Flags: approval-mozilla-esr78?
Attachment #9167396 - Flags: approval-mozilla-esr78+
Attachment #9167396 - Flags: approval-mozilla-beta?
Attachment #9167396 - Flags: approval-mozilla-beta+

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.

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.

Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: