If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 49

Status

()

Firefox
General
P4
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks: 1 bug)

42 Branch
Firefox 49
Points:
---

Firefox Tracking Flags

(e10s+, firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
tracking-e10s: --- → ?
(Assignee)

Comment 1

2 years ago
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.

Updated

2 years ago
tracking-e10s: ? → +
Priority: -- → P4

Updated

2 years ago
Blocks: 984139
(Assignee)

Comment 2

2 years ago
Created attachment 8740423 [details]
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/
Attachment #8740423 - Flags: review?(mak77)
(Assignee)

Comment 3

2 years ago
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... :-) )
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

Updated

2 years ago
Attachment #8740423 - Flags: review?(mak77)
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.
(Assignee)

Comment 5

2 years ago
(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?
Flags: needinfo?(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.
Flags: needinfo?(mconley) → needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 7

a year ago
(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...
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 8

a year ago
Created attachment 8749612 [details]
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/
Attachment #8749612 - Flags: review?(mconley)
(Assignee)

Updated

a year ago
Attachment #8740423 - Attachment is obsolete: true
Attachment #8749612 - Flags: review?(mconley) → review+
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

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

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/99a4b6cc8b08
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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]
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]
You need to log in before you can comment on or make changes to this bug.