1329032 - Firefox 53 shows blank white page and can't load about:plugins & about:serviceworkers links from about:support and about:about pages

Status: VERIFIED FIXED

(Core :: Security: CAPS, defect)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

STR:
1. Open about:support
2. Click on "about:plugins" or other "about:" links


Error shown in Browser Console:
> Security Error: Content at about:support may not load or link to about:plugins.


[Tracking Requested - why for this release]: Regression (recent one, probably since build [2017-01-05])

Comment 1

[Marcia Knous [:marcia]]

Some of these work for me on Mac using the latest nightly, and some don't-

These work:
* about:memory
* about:profiles
* about:buildconfig

These don't:
* about:plugins
* about:performance
* about:serviceworkers

We will track this regression for 53.

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9307d60e21ba70f46fb47c9eb43b6b5246b7e64c&tochange=d31df518cb6ec920cf9ab7accb67682298f8a9f8

Suspect:Bug 1182569

Comment 3

[:Gijs (he/him)]

I'm surprised this regressed as a result of Christoph's changes, and also surprised this didn't orange caps/tests/mochitest/browser_checkloaduri.js . Maybe a result of bug 1295543. :-(

Comment 4

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

This is definitely a problem caused by Bug 1182569.

Following the steps from comment 0, here is what happens, about:support is about to load about:plugins. Within the ContentSecurityManager we get the following arguments:

>  channelURI: about:plugins
>  triggeringPrincipal: about:support
>  contentPolicyType: 6
>  securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, 

The ContentSecurityManager performs a call to CheckLoadURIWithPrincipal. Important to note is that nsAboutProtocolhandler uses the following protocol flags:
> URI_NORELATIVE | URI_NOAUTH | URI_DANGEROUS_TO_LOAD | URI_SCHEME_NOT_SELF_LINKABLE;

So, within CheckLoadURIWIthPrincipal, the schemes are equal, but denySameSchemeLinks is true because of URI_SCHEME_NOT_SELF_LINKABLE and hence CheckLoadURIWithPrincipal consults CheckLoadURIFlags [1]. Since AboutProtocolhandler uses URI_DANGEROUS_TO_LOAD the load is denied by DenyAccessIfURIHasFlags within CheckLoadURIFlags [2].

Since we converted docshell loads to use asyncOpen2(), all docshell loads become subject to CheckLoadURIWithPrincipal checks which previously has not been enforced.

Boris, Olli do you know a way forward? about:support should be able to load about:plugins, right?

[1] https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#777
[2] https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#828

Comment 5

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> This is definitely a problem caused by Bug 1182569.
> 
> Following the steps from comment 0, here is what happens, about:support is
> about to load about:plugins. Within the ContentSecurityManager we get the
> following arguments:
> 
> >  channelURI: about:plugins
> >  triggeringPrincipal: about:support

But about:support is privileged. Why isn't this system principal, which should bypass all the other checks you're mentioning?

Comment 6

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Marcia Knous [:marcia - use ni] from comment #1)
> [...]
> These don't:
> * about:performance [...]

In my case it works, only about:plugins & about:serviceworkers don't.

Comment 7

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to :Gijs from comment #5)
> But about:support is privileged. Why isn't this system principal, which
> should bypass all the other checks you're mentioning?

Ah, I haven't check this yet, but most likely because the actual triggeringPrincipal within the loadinfo is left empty and hence we create a triggeringPrincipal from the referrer [1]

[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1519

Comment 8

[Boris Zbarsky [:bzbarsky]]

I think the question in comment 5 is key.  How is the load being performed, exactly, that we have no triggering principal but do have a referrer?  Whatever is doing that should stop, in this case, so we'll correctly end up with a system triggering principal...

Comment 9

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1329032_about_load.patch

Let me summarize my findings:

First, the problem does not appear in non-e10s mode. The load uses the correct triggeringPrincipal (SystemPrincipal).

Further, I traced down what's going on, let's start with the entry point into docshell, which is nsDocShell::OnLinkClickSync() which gets the right triggeringPrincipal (SystemPrincipal) which then calls InternalLoad (still using the right triggeringPrincipal). Further down within InternalLoad we call browserChrome3->ShouldLoadURI() which takes a uri and a referrer but *no* triggeringPricnipal. Here we loose the triggeringPrincipal the first time, but I suppose we could extend ShouldLoadURI with an additional argument triggeringPrincipal, right?

We then consult E10SUtils.shouldLoadURI() which then figures out that about:plugins should not run in the same process and hence we call E10SUtils.redirectLoad() which would then have to set the triggeringPrincipal into the loadOptions which then calls back into browser.js:RedirectLoad which then actually would perform LoadInOtherProcess(). So far so good.

There are some intermidate hoops in between, but not that important because the principal is still passed along within the loadArguments (see patch comments for details). At some point however we end up within restoreTabContent (in ContentRestore.js) and here we would loose the principal again because we call webNavigation.loadURIWithOptions() which does not take a triggeringPrincipal. This is also hard to fix (see Bug 1316275) because there are so many entry points. Since we don't have a triggeringPrincipal we use the referrer as the fallback and create a principal from the referrer (about:support).

I don't know how to proceed - I am open to suggestions. Gijs, Boris, what do you think? I suppose it's hard to actually pass along the right triggeringPrincipal. I suppose we could clear the referrer if certain conditions are met? What do you think?

Comment 10

[:Gijs (he/him)]

Thanks for diving into this!

I effectively see 2 ways to fix this:

1) pass the principal along all the way
2) infer the correct principal from the referrer. Specifically, you can check the about: flags for about:support and deduce that it has system principal, and use that rather than the codebase principal. I would expect we already do this for e.g. chrome:// URIs as well?


If we think bug 1316275 is going to not be landing for a long time (specifically, past the post-53-to-aurora-merge in ~2 weeks), we could potentially use (2) as a stopgap, but I think we will eventually need to use (1) anyway (ie, it feels like we need the endpoints where we can supply a triggeringPrincipal anyway).

Orthogonally, we should ideally move more and more about: pages into the content process (approach 3, kind of) and de-privilege them, but based on the data in this bug I'm worried that we will do the wrong thing in other cases than just about: pages, which also necessitates (1).

How do we determine the referrer that gets passed into this chain of events? What happens if it's a data:, blob:, javascript: or other non-codebase URI?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> How do we determine the referrer that gets passed into this chain of events?

It's just the document URI of the document that started the load.

> What happens if it's a data:, blob:, javascript: or other non-codebase URI?

Then that's the referrer.  And creating a principal from it creates a codebase principal for that random URI, iirc.

In terms of the question in comment 9, I think the right answer is to have an API for doing loads and providing a principal, then threading the principal though to that API.  Anything else will be very fragile.  In this specific case, where we just want to bounce the load to a different process and otherwise make it look the same, passing along the principal really is the way to go.  We shouldn't have to be constrained by the fact that browserChrome3 is implemented in JS (if it were in C++, it could just use existing principal-taking APIs, assuming it got a principal to start with).

Comment 12

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #11)
> In terms of the question in comment 9, I think the right answer is to have
> an API for doing loads and providing a principal, then threading the
> principal though to that API.

Given yours and Gijs' response it seems we should fix:
> Bug 1316275 - Try to explicitly pass aTriggeringPrincipal to nsDocShell::LoadURI
which would also include extending browserChrome3->ShouldLoadURI() by a triggeringPrincipal.

Comment 13

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

But I doubt that fixing  Bug 1316275 is doable within the next 2 weeks before the merge though. Should we try or have a different approach for 53?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Why is fixing bug 1316275 in a limited way (e.g. adding a new API with a name like "temporaryLoadURIWithOptionsDoNotUse" or something just for browserChrome3 to use, as opposed to changing an existing API) not doable before the merge?  It seems like a much more straightforward and simpler approach than anything else I've seen proposed here...

Comment 15

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #14)
> Why is fixing bug 1316275 in a limited way (e.g. adding a new API with a
> name like "temporaryLoadURIWithOptionsDoNotUse" or something just for
> browserChrome3 to use, as opposed to changing an existing API) not doable
> before the merge?  It seems like a much more straightforward and simpler
> approach than anything else I've seen proposed here...

Even without a new API, why would adding an optional parameter and leaving it empty if we don't have one be an issue? We can keep whatever fallbacks we have now - the situation would be no worse than it is today, AIUI.

Comment 16

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1329032_about_load.patch

Boris, Gijs, thanks for your suggestions. I looked at it again and actually it makes sense to extend loadURIWithOptions with an argument triggeringPrincipal. In some cases we still pass null as the triggeringPrincipal, but as you both said, it's not any worth than it is right now.

It's still a complicated patch and potentially we could replace the one or the other null-triggeringPrincipal with an actual principal. I think we should all stare at it, but it fixes the problem described in comment 0.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2040ac9eec687907b8b02c691e97272323d02dfe

Comment 17

[:Gijs (he/him)]

Comment on attachment 8825832 [details] [diff] [review]
bug_1329032_about_load.patch

Review of attachment 8825832 [details] [diff] [review]:
-----------------------------------------------------------------

r- because of the e10s miss.

This should have a test.

::: devtools/client/responsive.html/browser/web-navigation.js
@@ +65,4 @@
>    },
>  
>    loadURIWithOptions(uri, flags, referrer, referrerPolicy, postData, headers,
> +                     baseURI, triggeringPrincipal) {

This looks wrong, why is the argument unused? I would also expect eslint to complain about this.

::: docshell/base/nsIWebNavigation.idl
@@ +292,5 @@
>                            in unsigned long  aReferrerPolicy,
>                            in nsIInputStream aPostData,
>                            in nsIInputStream aHeaders,
> +                          in nsIURI         aBaseURI,
> +                          in nsIPrincipal   aTriggeringPrincipal);

Can we mark this optional? That avoids touching literally *all* the callsites to explicitly pass null, and will also improve add-on compat.

::: toolkit/components/remotebrowserutils/RemoteWebNavigation.js
@@ +74,3 @@
>    },
>    loadURIWithOptions(aURI, aLoadFlags, aReferrer, aReferrerPolicy,
> +                     aPostData, aHeaders, aBaseURI, aTriggeringPrincipal) {

Same issue here as with devtools. You'll need to pass the triggering principal into the _sendMessage options arg, and then pick it back out again on the other end (the 'other end' is in browser-child.js)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8825832 [details] [diff] [review]
bug_1329032_about_load.patch

Just reviewing the docshell side...

I agree that this should be an optional argument; apart from that the various .idl/.cpp bits look fine for the quick-fix.

For the longer-term fix, I think we should strongly consider a more JS-friendly API here (in a followup) so we don't have to keep adding arguments and deciding whether they're optional or not, etc.  I was going to suggest we simply add a way to pass in an nsIDocShellLoadInfo, but then I realized that there's no reason to force JS consumers to create such a beast.  A plausible scriptable API for this would look like this in xpidl:

  void thisAPINeedsASaneName(in AString aURI, [optiona] in jsval aOptions);

plus a webidl dictionary definition that we use to pull things out of the aOptions object (throwing when a non-object is passed) and then create an nsIDocShellLoadInfo from on the C++ side.  A consumer of the API will then do:

  webnav.thisAPINeedsASaneName(myURI, { triggeringPrincipal: whatever });

etc, only passing in the options they want non-default behavior for.

Comment 19

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1329032_about_load.patch

(In reply to :Gijs from comment #17)
> ::: devtools/client/responsive.html/browser/web-navigation.js
> >    loadURIWithOptions(uri, flags, referrer, referrerPolicy, postData, headers,
> > +                     baseURI, triggeringPrincipal) {
> 
> This looks wrong, why is the argument unused? I would also expect eslint to
> complain about this.

The argument is unused because LoadURI() does not take a triggeringPrincipal as an argument. At least yet. Bug 1316275 will fix that. I added a comment. Same in RemoteWebNavigation.js.

Question: I was thinking about a test for that scenario. But in fact it's hard to write such a test (at least from my understanding). Would we really need to basically load about:support which then triggers a load to about:plugins so that we can then check the correct triggeringPrincipal on the about:plugins load? Or is there an easier alternative. If you can think of something, please let me know. I am more than happy to add a test for this.

Comment 20

[:Gijs (he/him)]

Comment on attachment 8826144 [details] [diff] [review]
bug_1329032_about_load.patch

Review of attachment 8826144 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #19)
> Created attachment 8826144 [details] [diff] [review]
> bug_1329032_about_load.patch
> 
> (In reply to :Gijs from comment #17)
> > ::: devtools/client/responsive.html/browser/web-navigation.js
> > >    loadURIWithOptions(uri, flags, referrer, referrerPolicy, postData, headers,
> > > +                     baseURI, triggeringPrincipal) {
> > 
> > This looks wrong, why is the argument unused? I would also expect eslint to
> > complain about this.
> 
> The argument is unused because LoadURI() does not take a triggeringPrincipal
> as an argument. At least yet. Bug 1316275 will fix that. I added a comment.
> Same in RemoteWebNavigation.js.

I don't think this is true. The other end (the "WebNavigation" object in browser-child.js that really isn't an nsIWebNavigation) calls this.loadURI(), which isn't the LoadURI you're talking about, but just some JS method, specifically:

https://dxr.mozilla.org/mozilla-central/rev/b1c31c4a0a678194931779e0f13fba7b508eb109/toolkit/content/browser-child.js#312

which calls loadURIWithOptions on a 'real' nsIWebNavigation, like everything else:

https://dxr.mozilla.org/mozilla-central/rev/b1c31c4a0a678194931779e0f13fba7b508eb109/toolkit/content/browser-child.js#333-334

so we should update this path in this patch, too.

> Question: I was thinking about a test for that scenario. But in fact it's
> hard to write such a test (at least from my understanding). Would we really
> need to basically load about:support which then triggers a load to
> about:plugins so that we can then check the correct triggeringPrincipal on
> the about:plugins load? Or is there an easier alternative. If you can think
> of something, please let me know. I am more than happy to add a test for
> this.

I can't think of something much easier, no. I actually think that ideally we shouldn't need to rely on about:plugins or about:support as their remoteness stuff might change (or the pages might disappear altogether, which seems particularly likely for about:plugins now that NPAPI support is on its last legs).

I think what would work is a browser test that creates a test-only about: module that loads in the parent, and one that loads in the child, and then putting a link in the page on the parent, clicking it, and verifying that that load gets passed the correct triggeringPrincipal.

For an example of dynamically creating about:  modules that force loading in the parent/child, and registering/unregistering them (which you could do in the test) see https://dxr.mozilla.org/mozilla-central/source/browser/extensions/pocket/content/AboutPocket.jsm . Can probably copy/paste large portions of that into helpers in e.g. browsertestutils. There are other cases where I've wanted to do this (e.g. bug 1295543 ). If this seems daunting, I'd also be happy for you to write the test against about:support and about:plugins, and I can update it to use test-only about: URIs.

(FWIW, I agree with bz that it'd be great if we could just pass a JS {} with relevant properties, which would do wonders for backwards-compat and the verbosity of some of our internal APIs. Can we make sure a followup gets filed for that?)

Comment 21

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1329032_about_load.patch

(In reply to :Gijs from comment #20)
> I don't think this is true. The other end (the "WebNavigation" object in
> browser-child.js that really isn't an nsIWebNavigation) calls
> this.loadURI(), which isn't the LoadURI you're talking about, but just some
> JS method, specifically:

I looked at that part again, and you are absolutely right. I updated it within this patch - thanks for pointing that out.

Comment 22

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1329032_about_load_test.patch

I don't exactly what's not working with the test, but let me summarize what we have so far. The about page in the parent loads, we then click the link which loads another about page in the child which also loads but spits out the following error (see underneath) and then the test hangs...

JavaScript error: resource://gre/modules/RemoteAddonsChild.jsm, line 240: NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsINetworkInterceptController.shouldPrepareForIntercept]
4 INFO Console message: [JavaScript Error: "NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsINetworkInterceptController.shouldPrepareForIntercept]" {file: "resource://gre/modules/RemoteAddonsChild.jsm" line: 240}]
JavaScript error: resource://gre/modules/RemoteAddonsChild.jsm, line 240: NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]
JavaScript error: resource://gre/modules/RemoteAddonsChild.jsm, line 240: NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]
5 INFO Console message: [JavaScript Error: "NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]" {file: "resource://gre/modules/RemoteAddonsChild.jsm" line: 240}]
6 INFO Console message: [JavaScript Error: "NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]" {file: "resource://gre/modules/RemoteAddonsChild.jsm" line: 240}]
JavaScript error: resource://gre/modules/RemoteAddonsChild.jsm, line 240: NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]
JavaScript error: resource://gre/modules/RemoteAddonsChild.jsm, line 240: NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]
7 INFO Console message: [JavaScript Error: "NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]" {file: "resource://gre/modules/RemoteAddonsChild.jsm" line: 240}]
8 INFO Console message: [JavaScript Error: "NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]" {file: "resource://gre/modules/RemoteAddonsChild.jsm" line: 240}]
JavaScript error: resource://gre/modules/RemoteAddonsChild.jsm, line 240: NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIProgressEventSink.onStatus]
JavaScript error: resource://gre/modules/RemoteAddonsChild.jsm, line 240: NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIProgressEventSink.onStatus]
JavaScript error: resource://gre/modules/RemoteAddonsChild.jsm, line 240: NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIProgressEventSink.onStatus]
JavaScript error: resource://gre/modules/RemoteAddonsChild.jsm, line 240: NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIProgressEventSink.onStatus]
JavaScript error: resource://gre/modules/RemoteAddonsChild.jsm, line 240: NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIProgressEventSink.onStatus]
9 INFO Console message: [JavaScript Error: "NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIProgressEventSink.onStatus]" {file: "resource://gre/modules/RemoteAddonsChild.jsm" line: 240}]
10 INFO Console message: [JavaScript Error: "NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIProgressEventSink.onStatus]" {file: "resource://gre/modules/RemoteAddonsChild.jsm" line: 240}]
11 INFO Console message: [JavaScript Error: "NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIProgressEventSink.onStatus]" {file: "resource://gre/modules/RemoteAddonsChild.jsm" line: 240}]
12 INFO Console message: [JavaScript Error: "NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIProgressEventSink.onStatus]" {file: "resource://gre/modules/RemoteAddonsChild.jsm" line: 240}]
13 INFO Console message: [JavaScript Error: "NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIProgressEventSink.onStatus]" {file: "resource://gre/modules/RemoteAddonsChild.jsm" line: 240}]
JavaScript error: resource://gre/modules/RemoteAddonsChild.jsm, line 240: NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIProgressEventSink.onStatus]
JavaScript error: resource://gre/modules/RemoteAddonsChild.jsm, line 240: NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIProgressEventSink.onProgress]
14 INFO Console message: [JavaScript Error: "NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIProgressEventSink.onStatus]" {file: "resource://gre/modules/RemoteAddonsChild.jsm" line: 240}]
15 INFO Console message: [JavaScript Error: "NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIProgressEventSink.onProgress]" {file: "resource://gre/modules/RemoteAddonsChild.jsm" line: 240}]

Comment 24

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

FYI, about:plugins & about:serviceworkers links are also affected in about:about

Comment 25

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1329032_about_load_test.patch

Since you haven't reviewed yet, here is a rebased patch on top of central.

Comment 26

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #25)
> Created attachment 8827409 [details] [diff] [review]
> bug_1329032_about_load_test.patch
> 
> Since you haven't reviewed yet, here is a rebased patch on top of central.

Oh sorry, this is the test, it's working now. The problem was indeed: Services.ppmm.loadProcessScript. Thanks for your help with that test!

Comment 27

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1329032_about_load.patch

This is the rebased code-patch on top of central now.

Comment 28

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

I haven't pushed it to TRY yet with the added test, I assume it will take one more iteration before the test is final.

Comment 29

[:Gijs (he/him)]

Comment on attachment 8827410 [details] [diff] [review]
bug_1329032_about_load.patch

Review of attachment 8827410 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +869,5 @@
>        }
>  
>        browser.webNavigation.loadURIWithOptions(uri, flags,
>                                                 referrer, referrerPolicy,
> +                                               postData, null, null, null);

This can stay the same given that it's optional, right?

@@ +904,5 @@
>          browser.webNavigation.setOriginAttributesBeforeLoading({ userContextId: params.userContextId });
>        }
>  
>        browser.webNavigation.loadURIWithOptions(uri, flags, referrer, referrerPolicy,
> +                                               postData, null, null, null);

Same.

@@ +4372,5 @@
>      // Ignore loads that aren't in the main tabbrowser
>      if (browser.localName != "browser" || !browser.getTabBrowser || browser.getTabBrowser() != gBrowser)
>        return true;
>  
> +    if (!E10SUtils.shouldLoadURI(aDocShell, aURI, aReferrer, aTriggeringPrincipal)) {

So you'd omit the aTriggeringPrincipal for this call into E10SUtils.

::: browser/base/content/tab-content.js
@@ +704,5 @@
>    },
>  
>    // Check whether this URI should load in the current process
> +  shouldLoadURI(aDocShell, aURI, aReferrer, aTriggeringPrincipal) {
> +    if (!E10SUtils.shouldLoadURI(aDocShell, aURI, aReferrer, aTriggeringPrincipal)) {

And omit the triggering principal from here.

::: browser/modules/E10SUtils.jsm
@@ +148,5 @@
>      let remoteType = Services.appinfo.remoteType;
>      return remoteType == this.getRemoteTypeForURI(aURI.spec, true, remoteType);
>    },
>  
> +  shouldLoadURI(aDocShell, aURI, aReferrer, aTriggeringPrincipal) {

Why are we passing the triggering principal here, given that we don't do anything with it? To be clear, the nsIWebBrowserChrome3 implementation needs this argument, but this method, though it has the same name, doesn't seem to need it AFAICT...

::: docshell/base/nsIWebNavigation.idl
@@ +283,5 @@
>     *        that at present this argument is only used with view-source aURIs
>     *        and cannot be used to resolve aURI.
>     *        This parameter is optional and may be null.
> +   * @param aTriggeringPrincipal
> +   *        The principal that initiated the load of aURI.

Can we add documentation here that indicates what happens if you don't pass this?

::: toolkit/components/remotebrowserutils/RemoteWebNavigation.js
@@ +71,5 @@
>    },
>    loadURI(aURI, aLoadFlags, aReferrer, aPostData, aHeaders) {
>      this.loadURIWithOptions(aURI, aLoadFlags, aReferrer,
>                              Ci.nsIHttpChannel.REFERRER_POLICY_UNSET,
> +                            aPostData, aHeaders, null, null);

Doesn't look like this needs changing?

::: toolkit/components/viewsource/content/viewSource-content.js
@@ +391,5 @@
>          docShell.QueryInterface(Ci.nsIWebNavigation)
>                  .loadURIWithOptions(content.location.href,
>                                      Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CLASSIFIER,
>                                      null, Ci.nsIHttpChannel.REFERRER_POLICY_UNSET,
> +                                    null, null, null, null);

Shouldn't need to change this one

::: toolkit/content/widgets/browser.xml
@@ +149,5 @@
>  
>              this._wrapURIChangeCall(() =>
>                this.webNavigation.loadURIWithOptions(
>                    aURI, aFlags, aReferrerURI, aReferrerPolicy,
> +                  aPostData, null, null, null));

This doesn't look like it needs changing.

Comment 30

[:Gijs (he/him)]

Comment on attachment 8827409 [details] [diff] [review]
bug_1329032_about_load_test.patch

Review of attachment 8827409 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser.ini
@@ +507,5 @@
>  skip-if = (os == "linux" && !e10s) # Bug 1263254 - Perma fails on Linux without e10s for some reason.
>  [browser_bug1299667.js]
>  [browser_close_dependent_tabs.js]
>  skip-if = !e10s # GroupedSHistory is e10s-only
> +[browser_e10s_about_page_triggeringprincipal.js]

Nit: sort this into the list

::: browser/base/content/test/general/browser_e10s_about_page_triggeringprincipal.js
@@ +1,5 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim:set ts=2 sw=2 sts=2 et: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

We normally license tests public domain, for which you need no license header at all.

@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * Description of the test

I think you can omit the "Description of the test" bit :-)

@@ +19,5 @@
> +const UNREGISTER_URL = "chrome://mochitests/content/browser/browser/base/content/test/general/file_unregister_about_page.js"
> +
> +registerCleanupFunction(() => {
> +  Services.ppmm.removeDelayedProcessScript(
> +    "chrome://mochitests/content/browser/browser/base/content/test/general/file_unregister_about_page.js"

removeDelayedProcessScript removes the script you added earlier (to avoid adding it to new child processes if any start up). So you need to pass the same argument.

@@ +24,5 @@
> +  );
> +});
> +
> +add_task(function* test_principal() {
> +

Nit: no empty lines at the start/end of blocks.

@@ +33,5 @@
> +
> +  yield BrowserTestUtils.withNewTab({
> +    gBrowser,
> +    url: "about:test-about-principal-parent"
> +  },

Nit: I know the docs are unclear, but you can just do:

....withNewTab("about:test-about-principal-parent", function*(browser) {

(ie you don't need to explicitly pass gBrowser and the URL in an object if you're using the current browser, you can just only pass the URL as a string)

@@ +35,5 @@
> +    gBrowser,
> +    url: "about:test-about-principal-parent"
> +  },
> +  function*(browser) {
> +

Nit: no empty lines at the start/end of blocks.

@@ +42,5 @@
> +    myLink.click();
> +    yield loadPromise;
> +
> +    yield ContentTask.spawn(gBrowser.selectedBrowser, {}, function*() {
> +

Ditto.

@@ +49,5 @@
> +      is(channel.originalURI.asciiSpec,
> +         "about:test-about-principal-child",
> +         "sanity check - make sure we test the principal for the correct URI");
> +
> +      var triggeringPrincipal = channel.loadInfo.triggeringPrincipal;

Nit: 'let' instead of 'var' here and below.

@@ +60,5 @@
> +
> +      var loadingPrincipal = channel.loadInfo.loadingPrincipal;
> +      is(loadingPrincipal, null,
> +         "sanity check - load of TYPE_DOCUMENT must have a null loadingPrincipal");
> +

Nit: No empty line.

::: browser/base/content/test/general/file_register_about_page.js
@@ +73,5 @@
> +                "About Principal Parent Test",
> +                Ci.nsIAboutModule.ALLOW_SCRIPT)
> +);
> +
> +this.EXPORTED_SYMBOLS = ["AboutPrincipalTest"];

I think you don't need this.

@@ +77,5 @@
> +this.EXPORTED_SYMBOLS = ["AboutPrincipalTest"];
> +
> +
> +AboutPrincipalTest.aboutParent.register();
> +AboutPrincipalTest.aboutChild.register();

So, instead of the separate file, I think you can do roughly the following:


function onUnregisterMessage() {
  removeMessageListener("AboutPrincipalTest:Unregister", onUnregisterMessage);
  AboutPrincipalTest.aboutParent.unregister();
  AboutPrincipalTest.aboutChild.unregister();
  sendAsyncMessage("AboutPrincipalTest:Unregistered");
}

addMessageListener("AboutPrincipalTest:Unregister", onUnregisterMessage);


In the test's registered cleanup function, before you unload the process script, do something like:

sendAsyncMessage("AboutPrincipalTest:Unregister");
yield BrowserTestUtils.waitForMessage(Services.ppmm, "AboutPrincipalTest:Unregistered");
// then unload the process script.

::: browser/base/content/test/general/file_unregister_about_page.js
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +AboutPrincipalTest.aboutParent.unregister();
> +AboutPrincipalTest.aboutChild.unregister();

I would not expect this to work when run, as AboutPrincipalTest is not defined in here.

Comment 31

[:Gijs (he/him)]

(In reply to :Gijs from comment #30)
> In the test's registered cleanup function, before you unload the process
> script, do something like:
> 
> sendAsyncMessage("AboutPrincipalTest:Unregister");
> yield BrowserTestUtils.waitForMessage(Services.ppmm,
> "AboutPrincipalTest:Unregistered");
> // then unload the process script.

Err, sendAsyncMessage should be Services.ppmm.sendAsyncMessage


the message listeners I mentioned should be in the main child process script.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Have we also tested what happens if you use "open in new tab" from the context menu on the relevant links?  It's broken without this patch; does the patch fix it?

Comment 33

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #32)
> Have we also tested what happens if you use "open in new tab" from the
> context menu on the relevant links?  It's broken without this patch; does
> the patch fix it?

Without patch I get this error in Browser Console and same end behavior

> NS_ERROR_DOM_BAD_URI: Component returned failure code: 0x805303f4 [nsIWebNavigation.loadURIWithOptions]
> browser-child.js:333

Comment 34

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #32)
> Have we also tested what happens if you use "open in new tab" from the
> context menu on the relevant links?  It's broken without this patch; does
> the patch fix it?

Thanks for letting me know!

So far:
a) Opening about:support and clicking on about:plugins works
b) Opening about:support and CTRL+CLICK on about:plugins does not work
   -> Security Error: Content at about:support may not load or link to about:plugins.
c) Opening about:support and right-click->open in new tab does not work
   -> Security Error: Content at about:support may not load or link to about:plugins.
d) Opening about:support and right-click-> open in new window does not work
   -> Security Error: Content at about:support may not load or link to about:plugins.
e) Opening about:support and drag/drop about:plugins into a new tab works

I guess there is more things we have to fix, I'll look into that right away!

Comment 35

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1329032_about_load.patch

Incorporated Gijs suggestions and carrying over r+ on that patch! (Probably more to come)

Comment 36

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1329032_about_load_test.patch

Incorporated Gijs suggestions. Carrying over r+ for those tests.

Comment 37

[Boris Zbarsky [:bzbarsky]]

> b) Opening about:support and CTRL+CLICK on about:plugins does not work

OK.  Note that it's probably fine to fix that in a followup (still blocking 1182569 and tracked for 53, of course).

Your cases b, c, d all take the same codepath in the end, as I recall, though they have slightly different entrypoints into it.  So fixing one of them will go a long way toward fixing them all.

Comment 38

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #37)
> > b) Opening about:support and CTRL+CLICK on about:plugins does not work
> 
> OK.  Note that it's probably fine to fix that in a followup (still blocking
> 1182569 and tracked for 53, of course).
> 
> Your cases b, c, d all take the same codepath in the end, as I recall,
> though they have slightly different entrypoints into it.  So fixing one of
> them will go a long way toward fixing them all.

Yeah, I think all of these go through https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#846 in the end, and all/most probably go through openLinkIn and friends in utilityOverlay.js .

I agree with bz that it might be helpful to move that to a separate bug, as the patches here create the relevant APIs that we'll then end up building on anyway.

Comment 39

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bcd68af323a2ce0eba8df4cb44d78cdf70df1e4

I filed Bug 1331686 to fix the follow ups from comment 34

Comment 40

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to :Gijs from comment #20)
> (FWIW, I agree with bz that it'd be great if we could just pass a JS {} with
> relevant properties, which would do wonders for backwards-compat and the
> verbosity of some of our internal APIs. Can we make sure a followup gets
> filed for that?)

Yes, I filed Bug 1331735

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54f7bfe3ca11
Extend loadURIWithOptions by a triggeringPrincipal. r=bz,gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/16c4f0df527d
Test privileged about page to use SystemPrincipal as TriggeringPrincipal when loading about page in child. r=gijs

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=69687634&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/56f0b3f7b125

Comment 43

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Wes Kocher (:KWierso) from comment #42)
> I had to back this out for failures like

Running ./mach eslint before pushing is a good idea. Thanks for the backout and sorry for the additional trouble.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a385d2425a76
Extend loadURIWithOptions by a triggeringPrincipal. r=bz,gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b544b8ab06b
Test privileged about page to use SystemPrincipal as TriggeringPrincipal when loading about page in child. r=gijs

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a385d2425a76
https://hg.mozilla.org/mozilla-central/rev/5b544b8ab06b

Comment 46

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm marking this bug as VERIFIED,
as it's fixed, started from 53.0a1 (2017-01-19).

Leftovers are being dealt in bug #1331686.