1249362 - Tabs can lack contentPrincipal in e10s when stopping an about:blank load from chrome

Status: RESOLVED FIXED

(Firefox :: General, defect, P4)

Description

[:Gijs (he/him)]

STR:

1. run this in the browser console:

var tab = gBrowser.addTab("about:blank", { skipAnimation: true }); tab.linkedBrowser.stop(); gBrowser.selectedTab = tab; alert(gBrowser.selectedBrowser.contentPrincipal);

ER:

a principal for about:blank

AR:

null


This bites us in code that relies on browsers having a principal in "normal" cases, and means we can't accurately determine what's happening in that tab.

While this is likely almost impossible to hit in practice through normal usage, mochitest-browser hits exactly this case:

https://dxr.mozilla.org/mozilla-central/rev/0629918a09ae87808efdda432d7852371ba37db6/testing/mochitest/browser-test.js#302-307

Comment 1

[:Gijs (he/him)]

I think this also causes these errors:

2 INFO TEST-INFO | (browser-test.js) | Console message: [JavaScript Error: "TelemetryStopwatch: requesting elapsed time for nonexisting stopwatch. Histogram: "FX_PAGE_LOAD_MS", key: "null"" {file: "resource://gre/modules/TelemetryStopwatch.jsm" line: 297}]
this.TelemetryStopwatchImpl.timeElapsed@resource://gre/modules/TelemetryStopwatch.jsm:297:7
this.TelemetryStopwatchImpl.finish@resource://gre/modules/TelemetryStopwatch.jsm:315:17
this.TelemetryStopwatch.finish@resource://gre/modules/TelemetryStopwatch.jsm:192:12
TabsProgressListener.onStateChange@chrome://browser/content/browser.js:4646:11
callListeners@chrome://browser/content/tabbrowser.xml:501:24
_callProgressListeners@chrome://browser/content/tabbrowser.xml:522:13
mTabProgressListener/<._callProgressListeners@chrome://browser/content/tabbrowser.xml:571:22
mTabProgressListener/<.onStateChange@chrome://browser/content/tabbrowser.xml:729:15
stop@chrome://global/content/bindings/browser.xml:100:13
stop@chrome://browser/content/tabbrowser.xml:3811:20
Tester_waitForWindowsState@chrome://mochikit/content/browser-test.js:303:7
Tester.prototype.nextTest<@chrome://mochikit/content/browser-test.js:591:5
TaskImpl_run@resource://gre/modules/Task.jsm:319:40
TaskImpl@resource://gre/modules/Task.jsm:280:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14
waitForGraphicsTestWindowToBeGone@chrome://mochikit/content/browser-test.js:278:5
Tester_start@chrome://mochikit/content/browser-test.js:262:7
createTester/</<@chrome://mochikit/content/browser-harness.xul:255:11

which show up in logs from mochitest-browser.

When we fix this, we should remove the workaround for this bug that I'm adding in bug 1228754.

Comment 2

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1249362 - correct content principal when about:blank loads stop, r?mconley

Review commit: https://reviewboard.mozilla.org/r/45753/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45753/

Comment 3

[:Gijs (he/him)]

Marco, looks like mconley is catching up on review requests. Let me know if you're not comfortable reviewing this, in which case I can find another victim... (probably Felipe... :-) )

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8740423 [details]
MozReview Request: Bug 1249362 - correct content principal when about:blank loads stop, r?mconley

https://reviewboard.mozilla.org/r/45753/#review43305

technically the patch is ok, but I'm not sure I understand its benefit.

I thought the scope of this bug was to remove the workaround in checkEmptyPageOrigin, but off-hand that would require a synchronous request we don't want.

The patch is changing a certain failure (we know the principal is always null in this case) with an uncertain failure (it may be null sometimes), and I'm not sure that's a win.
If you could provide more insight about the benefits of this change, that would be useful.

::: browser/base/content/browser.js:6330
(Diff revision 1)
>    let contentPrincipal = browser.contentPrincipal;
>    if (gMultiProcessBrowser && browser.isRemoteBrowser &&
>        !contentPrincipal && uri.spec == "about:blank") {
>      // Need to specialcase this because of how stopping an about:blank
> -    // load from chrome on e10s causes a permanently null contentPrincipal,
> -    // see bug 1249362.
> +    // load from chrome on e10s immediately leaves a null principal for
> +    // one turn of the event loop, see bug 1249362.

how can we tell it's just one turn, is it not until we get the message from the child?

::: toolkit/content/browser-child.js:134
(Diff revision 1)
>      if (aWebProgress && aWebProgress.isTopLevel) {
>        json.documentURI = content.document.documentURIObject.spec;
>        json.charset = content.document.characterSet;
>        json.mayEnableCharacterEncodingMenu = docShell.mayEnableCharacterEncodingMenu;
> +
> +      // Separately, if we're stopped, we also want to know the content principal:

I think this comment deservers to be expanded, for future reference. As it is, it's completely unclear WHY we want to know it.

Comment 5

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #4)
> ::: browser/base/content/browser.js:6330
> (Diff revision 1)
> >    let contentPrincipal = browser.contentPrincipal;
> >    if (gMultiProcessBrowser && browser.isRemoteBrowser &&
> >        !contentPrincipal && uri.spec == "about:blank") {
> >      // Need to specialcase this because of how stopping an about:blank
> > -    // load from chrome on e10s causes a permanently null contentPrincipal,
> > -    // see bug 1249362.
> > +    // load from chrome on e10s immediately leaves a null principal for
> > +    // one turn of the event loop, see bug 1249362.
> 
> how can we tell it's just one turn, is it not until we get the message from
> the child?

This is true.

> Comment on attachment 8740423 [details]
> MozReview Request: Bug 1249362 - correct content principal when about:blank
> loads stop, r?mconley
> 
> https://reviewboard.mozilla.org/r/45753/#review43305
> 
> technically the patch is ok, but I'm not sure I understand its benefit.
> 
> I thought the scope of this bug was to remove the workaround in
> checkEmptyPageOrigin, but off-hand that would require a synchronous request
> we don't want.
> 
> The patch is changing a certain failure (we know the principal is always
> null in this case) with an uncertain failure (it may be null sometimes), and
> I'm not sure that's a win.
> If you could provide more insight about the benefits of this change, that
> would be useful.

Yeah, this is more along the lines of "best I could come up with".

It's annoying to have this edgecase in the browser.js code, and part of me worries that it will come bite us in the ... at some point in the future.

I don't really see a good way to fix it to synchronously update after the stop call, besides, perhaps, prepopulating the contentPrincipal for a docshell with the about:blank one (in which case, actually, the question is - *which* about:blank principal? I think it'd be the null principal variant, but I'm not even 100% sure off-hand). But that feels hacky because I don't know that we can rely on what docshell loads initially (cf. bug 610357).

Maybe Mike has a better idea?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Marco Bonardo [::mak] from comment #4)
> > ::: browser/base/content/browser.js:6330
> > (Diff revision 1)
> > >    let contentPrincipal = browser.contentPrincipal;
> > >    if (gMultiProcessBrowser && browser.isRemoteBrowser &&
> > >        !contentPrincipal && uri.spec == "about:blank") {
> > >      // Need to specialcase this because of how stopping an about:blank
> > > -    // load from chrome on e10s causes a permanently null contentPrincipal,
> > > -    // see bug 1249362.
> > > +    // load from chrome on e10s immediately leaves a null principal for
> > > +    // one turn of the event loop, see bug 1249362.
> > 
> > how can we tell it's just one turn, is it not until we get the message from
> > the child?
> 
> This is true.
> 
> > Comment on attachment 8740423 [details]
> > MozReview Request: Bug 1249362 - correct content principal when about:blank
> > loads stop, r?mconley
> > 
> > https://reviewboard.mozilla.org/r/45753/#review43305
> > 
> > technically the patch is ok, but I'm not sure I understand its benefit.
> > 
> > I thought the scope of this bug was to remove the workaround in
> > checkEmptyPageOrigin, but off-hand that would require a synchronous request
> > we don't want.
> > 
> > The patch is changing a certain failure (we know the principal is always
> > null in this case) with an uncertain failure (it may be null sometimes), and
> > I'm not sure that's a win.
> > If you could provide more insight about the benefits of this change, that
> > would be useful.
> 
> Yeah, this is more along the lines of "best I could come up with".
> 
> It's annoying to have this edgecase in the browser.js code, and part of me
> worries that it will come bite us in the ... at some point in the future.
> 
> I don't really see a good way to fix it to synchronously update after the
> stop call, besides, perhaps, prepopulating the contentPrincipal for a
> docshell with the about:blank one (in which case, actually, the question is
> - *which* about:blank principal? I think it'd be the null principal variant,
> but I'm not even 100% sure off-hand). But that feels hacky because I don't
> know that we can rely on what docshell loads initially (cf. bug 610357).
> 
> Maybe Mike has a better idea?

Maybe this is a dumb idea, but can we not default to the Null Principal until we get a message from the child with the update? I assume it's likely better to have a Null Principal here than no principal at all.

Comment 7

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #6)
> Maybe this is a dumb idea, but can we not default to the Null Principal
> until we get a message from the child with the update? I assume it's likely
> better to have a Null Principal here than no principal at all.

WFM. I don't know what would break, if anything, but we could certainly go find out...

Comment 8

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1249362 - initialize remote browser's contentPrincipal to a null principal, r?mconley

Review commit: https://reviewboard.mozilla.org/r/51057/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51057/

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8749612 [details]
MozReview Request: Bug 1249362 - initialize remote browser's contentPrincipal to a null principal, r?mconley

https://reviewboard.mozilla.org/r/51057/#review47763

Looks good!

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99a4b6cc8b08

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/99a4b6cc8b08

Comment 12

[Kazi Nuzhat Tasnem]

I have reproduced this bug with Firefox nightly 47.0a1(build id:20160218030349)on
windows 7(64 bit)

I have verified this bug as fixed with Firefox aurora 49.0a2(build id:20160723004004)
User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

[testday-20160812]

Comment 13

[Farhadur Reja Fahim]

I have reproduced this bug with Nightly 47.0a1 (2016-02-18) on Ubuntu 14.04, 64 bit!

The bug's fix is now verified on latest Beta 49.0b3.

Beta 49.0b3:
Build ID 	20160811031722
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0

[bugday-20160817]