1806476 - webRequest.filterResponseData does not work when navigating from the "new" blank page, breaking uBlock Origin HTML filtering

Status: ASSIGNED

(WebExtensions :: Request Handling, defect, P2)

Description

[gwarser]

This should remove cookie consent dialog from Google search page in European region, so if you are outside, use VPN.

• set browser Home Page / New Tab to be blank
• install uBlock Origin'
• add google.com##^script:has-text(cookieUpdateConsentUrl) to "My filters"
• clear browsing data
• close browser
• open browser
• type or "paste and go" https://www.google.com/ in address bar

Expected:

Google page with input box.
There is no script element with cookieUpdateConsentUrl text in source.

Actual:

Consent dialog. script element with cookieUpdateConsentUrl text in source.

Reload page - dialog disappears. Script element is removed.

Bisected to:

36:56.35 INFO: Running autoland build built on 2022-11-04 15:16:50.808000, revision 2e7abeda
37:04.02 INFO: Launching /tmp/tmpw_al3ul3/firefox/firefox
37:04.02 INFO: Application command: /tmp/tmpw_al3ul3/firefox/firefox --allow-downgrade -profile /tmp/tmpbpmiuqvk.mozrunner
37:04.02 INFO: application_buildid: 20221101204539
37:04.02 INFO: application_changeset: 2e7abedae4a01ba3fefaa93462b5211824e5c947
37:04.02 INFO: application_name: Firefox
37:04.02 INFO: application_repository: https://hg.mozilla.org/integration/autoland
37:04.02 INFO: application_version: 108.0a1
Was this integration build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', 'back' or 'exit' and press Enter): bad
37:17.29 INFO: Narrowed integration regression window from [d6b82b3d, 6654b5c9] (3 builds) to [d6b82b3d, 2e7abeda] (2 builds) (~1 steps left)
37:17.29 INFO: No more integration revisions, bisection finished.
37:17.29 INFO: Last good revision: d6b82b3d655959b256be143218b86ee28d9fe06a
37:17.29 INFO: First bad revision: 2e7abedae4a01ba3fefaa93462b5211824e5c947
37:17.29 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d6b82b3d655959b256be143218b86ee28d9fe06a&tochange=2e7abedae4a01ba3fefaa93462b5211824e5c947

[rdk@on ~]$ 

https://bugzilla.mozilla.org/show_bug.cgi?id=1676492

Comment 1

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

Set release status flags based on info from the regressing bug 1676492

:enndeakin, since you are the author of the regressor, bug 1676492, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 2

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

We agreed that this is something we need to investigate sooner rather than later, needinfo-ing myself as a reminder.

Comment 3

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

I managed to look a little bit more deeply into this, the STR can reproduce the issue consistently and I verified that:

• on the extension process side, uBlock origin will be creating a StreamFilter instance through webRequest.filterResponseData, and that instance is then getting an "error" event with the generic "Invalid request ID" error message (which is actually used for any of the rejection originated from StreamFilterParent on the parent process side).

• on the parent process side:

  • StreamFilterParent::Create is called and the channel for the document being loaded is found as expected and we end up to call nsHttpChannel::AttachStreamFilter
  • In nsHttpChannel::AttachStreamFilter the call is returning immediately and rejecting the request because nsHttpChannel::ProcessId() is 0

That seems to be due to the fact that the browser element for the new about:newtab blank page ends up to be remote="false" and that seems to be happening because:

• after getting redirected to "chrome://browser/content/blanktab.html", we get to call DocumentLoadListener::MaybeTriggerProcessSwitch and to determine if we should trigger a process switch we end up calling mozilla::dom::IsolationBehaviorForURI where we are returning IsolationBehavior::Parent (through mozilla::dom::IsolationOptionsForNavigation)

Checking what the chrome registry canLoadURLRemotely and mustLoadURLRemotely are returning for "chrome://browser/content/blanktab.html" from the BrowserConsole confirm that is false in both cases and so expected that mozilla::dom::IsolationBehaviorForURI would be returning IsolationBehavior::Parent .

This doesn't happen for about:welcome, etc. , I guess because those are not translating to a chrome url and so they are getting loaded in privilegeabout child processes, and so the browser element is remote="true".

Hey Neal,
does the sequence describe above match what you would expect to happen given the details provided?

I was also wondering if loading "chrome://browser/content/blanktab.html" in the parent process is something that have been done on purpose or we may have missed to set flags to make sure we allowed it to switch the browser to remote="true"?

Comment 4

[Rob Wu [:robwu]]

FYI there is some existing discussion on this other bug about extensions broken by this change: bug 1800995.

Comment 5

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

Hi Gijs,
This regressions seems to be breaking adblocking on the first load on a new tab if the new tab url is set to the new blank tab url,
in comment 3 I added some details I gathered while investigating this, but there are some doubts about what I've observed that I wanted to confirm with someone that has some more direct knowledge about those internals so that then we can agree on how to fix the regression properly (as a side note. I haven't double-checked yet if Bug 1800995 is due to the exact same underlying behavior described in comment 3, but it is not unlikely that may be the case).

I noticed that you have reviewed the patch that introduced the new blank tab page, would you be able to help us answering the questions at the bottom of comment 3 if Neal is currently too busy to take a look into that? (or redirect the needinfo to someone that may have some time to look into that)

Comment 6

[:Gijs (he/him)]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #5)

Hi Gijs,
This regressions seems to be breaking adblocking on the first load on a new tab if the new tab url is set to the new blank tab url,
in comment 3 I added some details I gathered while investigating this, but there are some doubts about what I've observed that I wanted to confirm with someone that has some more direct knowledge about those internals so that then we can agree on how to fix the regression properly (as a side note. I haven't double-checked yet if Bug 1800995 is due to the exact same underlying behavior described in comment 3, but it is not unlikely that may be the case).

I noticed that you have reviewed the patch that introduced the new blank tab page, would you be able to help us answering the questions at the bottom of comment 3 if Neal is currently too busy to take a look into that? (or redirect the needinfo to someone that may have some time to look into that)

Mostly I'm confused. On current nightly if I set it to blank new tab pages, open a new tab, and evaluate gBrowser.selectedBrowser.isRemoteBrowser it says true.

So I don't understand why you're finding the tab loading in the parent process. That shouldn't be happening, and doesn't seem to for me.

bug 1800995 appears to be related to whether webextension code can run on a page with privileged API access, so that sounds different to the root cause you have so far - but then, I'm confused about that root cause so 🤷‍♂️

Comment 7

[:Gijs (he/him)]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #3)

Checking what the chrome registry canLoadURLRemotely and mustLoadURLRemotely are returning for "chrome://browser/content/blanktab.html" from the BrowserConsole confirm that is false in both cases and so expected that mozilla::dom::IsolationBehaviorForURI would be returning IsolationBehavior::Parent .

This doesn't happen for about:welcome, etc. , I guess because those are not translating to a chrome url and so they are getting loaded in privilegeabout child processes, and so the browser element is remote="true".

FWIW for this thing, AIUI chrome URIs never return true for those APIs, but about URIs do, even if they point to chrome ones, e.g. E10SUtils.getRemoteTypeForURI("about:certviewer", true, true) returns web, even though it points to a chrome URI.

Comment 8

[:Gijs (he/him)]

(Fundamentally, it seems bad that the remoteness of the current page would impact whether we can run webextension code for a subsequent pageload, irrespective of what blank tabs are doing. So I feel like I'm missing something here, I just don't understand what it is, from comment 3)

Comment 9

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

(In reply to :Gijs (he/him) from comment #6)

...
Mostly I'm confused. On current nightly if I set it to blank new tab pages, open a new tab, and evaluate gBrowser.selectedBrowser.isRemoteBrowser it says true.

So I don't understand why you're finding the tab loading in the parent process. That shouldn't be happening, and doesn't seem to for me.

Based on what I recall (and unfortunately I missed to mention that explicitly in comment 3) the part of the STR about restarting the browser was necessary to be able to hit the issue, which is making me thing that there may be something different in the way the first blank tab was getting loaded that may be worth digging into.

Did you also restarted the browser when you tried to trigger this issue locally?

I'm going to also give it another try to refresh my memory and double-check if anything changed in that behavior in the meantime.

Comment 10

[:Gijs (he/him)]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #9)

Did you also restarted the browser when you tried to trigger this issue locally?

To be clear, I haven't tried to trigger the full issue. But yes, even after restarting the browser isRemoteBrowser is true, and remoteType on the browser says privilegedabout, for (blank) new tabs. If I instead load about:config, which is intentionally loaded in the parent, then isRemoteBrowser is false, and remoteType is null.

Comment 11

[Pascal Chevrel (relman team) -> :pascalc]

Is there any progress on the investigation? Could we have something for the 110 planned dot release? Thanks

Comment 12

[Pascal Chevrel (relman team) -> :pascalc]

Is that really a P1/S2?

Comment 13

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

(In reply to Pascal Chevrel:pascalc from comment #12)

Is that really a P1/S2?

We initially set it to P1/S2 because it looks like a legit regression and we wanted to prioritize better assessing the impact for the user.

Given that the setting that would make the issue to be hit is an opt-in setting and that it seems to affect only the first load in the new blank tab, S4 should be a more reasovable severity level (and P2 as the priority, given that we still want to pinpoint what is the actual underlying issue).

Comment 14

[Elle Solomina (she/her)]

Any progress here?