774633 - Ensure that the compartment principal always matches the document principal

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

This bug is a combination of a number of issues that I started digging into with bug 771354. I haven't totally worked out which parts are s-s and which aren't, but for now I'm just going to write a patch stack to clean this all up.

The first issue is that we currently override the document principal when the document is being loaded in a chrome docshell. This is noticeable (a) with remote XUL, (b) when loading a transient about:blank window before loading a chrome document, and (c) if a stupid extension loads a content document in a type="chrome" iframe.

Given the recent principal-from-compartment changes, (a) causes us to crash when loading XUL from file:// URIs (currently hidden behind a pref, but used in marionette).

(b) is a bit complex. In the common case for chrome windows, we load an about:blank document (which has an about:blank principal) before loading a chrome document, so the principal-override behavior described in the bug ensures that the compartment is created with system principal (and that we don't have to switch to system principal later). So when we just pass the document principal to the compartment directly, this causes the compartment to initially use about:blank for a principal, and switch principals later on with the JS_SetCompartmentPrincipals call. this is mostly fine, but worries me a bit because we don't currently recompute cross-compartment wrappers with JS_SetCompartmentPrincipals (this API was added in bug 764389). I think we should, and will add a patch to do that. Even when we do that though, there's still the issue of same-compartment security wrappers (Components and Location), which we currently don't have the capacity to recompute. I'll fix that in another patch here.

(c) would be an sg:crit.

Comment 1

[Bobby Holley (:bholley)]

So, in one of my patches here I added an assertion that we never change compartment principals from chrome to non-chrome, since that has the potential to break same-compartment security wrappers. However, we hit that assertion with remote XUL. I need to think about the best way to proceed here.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #1)
> So, in one of my patches here I added an assertion that we never change
> compartment principals from chrome to non-chrome, since that has the
> potential to break same-compartment security wrappers. However, we hit that
> assertion with remote XUL. I need to think about the best way to proceed
> here.

I think the answer here is to kill that mysterious condition in WouldReuseInnerWindow where we unconditionally reuse the inner window for about:blanks in nsGlobalChromeWindow.

Let's see if that breaks anything:
https://tbpl.mozilla.org/?tree=Try&rev=e999c594d94f

Comment 3

[Daniel Veditz [:dveditz]]

Isn't (c) a critical problem in the add-on just by definition, even apart from any bugs we have? Bad enough (and common enough) that the AMO reviewers specifically look for that case.

Taking a SWAG at sec-high for the rest of it?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Bobby Holley (:bholley) from comment #2)
> I think the answer here is to kill that mysterious condition in
> WouldReuseInnerWindow where we unconditionally reuse the inner window for
> about:blanks in nsGlobalChromeWindow.

IIRC, that was necessary so that chrome code could do the equivalent of:

w = window.open();
w.onload = function() { ... }
w.location = "chrome://...";

If we didn't reuse the inner window, I think the onload handler would get blown away. That being said, I think the last time I looked into this was close to 5 years ago, so I could be misremembering.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #4)
> (In reply to Bobby Holley (:bholley) from comment #2)
> > I think the answer here is to kill that mysterious condition in
> > WouldReuseInnerWindow where we unconditionally reuse the inner window for
> > about:blanks in nsGlobalChromeWindow.
> 
> IIRC, that was necessary so that chrome code could do the equivalent of:
> 
> w = window.open();
> w.onload = function() { ... }
> w.location = "chrome://...";
> 
> If we didn't reuse the inner window, I think the onload handler would get
> blown away. That being said, I think the last time I looked into this was
> close to 5 years ago, so I could be misremembering.

But this should still be possible in the common case, because the about:blank document in the window inherits the principal of its owner, which should be chrome in this case. So the same-origin check in WouldReuseInnerWindow should succeed.

I think the source of a lot of problems here comes from some weirdness with nsWindowWatcher which I'm looking into now.

Comment 6

[Bobby Holley (:bholley)]

Ok, so this just keeps getting more fun.

So. We need to kill the code that unconditionally reuses about:blank inner windows when the window is nsGlobalChromeWindow, because sometimes we load content in chrome windows, and that causes us to fail our assertions when updating compartment principals.

However, this causes certain tests to break (toolkit/mozapps/downloads/tests/chrome/test_destinationURI_annotation.xul is one that I'm looking at). Specifically, the case when chrome does:

var win = ww.openWindow(someContentWindow, "chrome://foo.xul", null, "chrome,centerscreen,titlebar,dialog=yes,dependent", null);
win.expando = 'bar';

(from nsHelperAppDlg.js:158)

This ends up creating an about:blank window with the inherited principal from |someContentWindow|. When the async load for chrome://foo.xul happens, we previously reused the inner window (since it was type=chrome), but now we don't (because the inherited content principal is not same-origin with the system principal we get from chrome://foo.xul). So win.expando is blown away.

The principal is very explicitly inherited from parent window by means of a cx push. This comes from bug 289204, where the chrome pop-up blocker would do contentWindow.open(blockedPopupURI), which was a bad place to use system principal of blockedPopupURI was a javascript: URI.

So our options here are to somehow make the inherited principal be chrome in the cases we care about, or to reuse the inner window in the cases we care about.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #6)
> So our options here are to somehow make the inherited principal be chrome in
> the cases we care about, or to reuse the inner window in the cases we care
> about.

I fixed this particular problem by special-casing type="chrome" windows. When type="content", you get the principal of the parent. When type="chrome", you get the principal of the caller.

Comment 8

[Bobby Holley (:bholley)]

I _think_ I've got this all sorted out. Pushing to try with fingers crossed:

https://tbpl.mozilla.org/?tree=Try&rev=6c04b8b1ee96

Comment 9

[Bobby Holley (:bholley)]

The last try push was borked by an incorrect assertion. I pushed again to try last night and got this:

https://tbpl.mozilla.org/?tree=Try&rev=9522ffa329e9

So pretty green, modulo one browser-chrome test failure (that cascades into others). After investigating it, I determined that the problem was the ordering of notifications nsAppShellService::RegisterTopLevelWindow. In particular, we tell the window mediator about the window _before_ telling the window watcher, so anybody listening to notifications via the window mediator runs into same issue I was running into with domwindowopened, whereby the listener is invoked before the inner window is set up with the correct principal, meaning that we blow the inner window away shortly thereafter.

This _could_ be fixed by just reordering those notifications, but that's scary and brittle. So I'm adding an API into nsGlobalWindow to do this principal setting directly, and then we can just call that from AppShell and the appropriate other places.

Comment 10

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=4eee555f1ec1

Comment 11

[Bobby Holley (:bholley)]

All_right_! Try is 100% green. Uploading patches and flagging review.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 1 - Factor nsWindowWatcher call to SetOpenerScriptPrincipal into a method on nsGlobalWindow. v1

This doesn't change any functionality in the code. Note that the name is currently
a bit of a misnomer, but we change that in the next patch when we rip out the
arguments.

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 2 - Wean SetInitialPrincipalToSubject off some parameters and clean it up. v1

We can remove the fallback to the principal of aParent because we already push the
cx for aParent (callerContextGuard) whenever we're concerned about using its principal.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove dead mOpenerScriptPrincipal. v1

This value is initialized to null and never set, so we can get rid of it.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 4 - Fold SetOpenerScriptPrincipal into SetInitialPrincipalToSubject, and make it indempotent and callable without an existing document. v1

There's no reason it has to fail if there's no mDoc, since any document is promptly
blown away with the new about:blank document.

The indempotence is important because we want to be able to call this method
unconditionally in OpenJSWindowInternal (since we may not have gone through
RegisterTopLevelWindow) without worrying about whether we've called it already.

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 5 - Move the call to SetInitialPrincipalToSubject into nsAppShellService::RegisterTopLevelWindow. v1

This means that we get the correct principal on the window before sending notifying
any consumers about the window's creation.

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 6 - Remove "is chrome window" condition for inner window reuse. v1

WouldReuseInnerWindow also returns true if the new window is same-origin with
the old one about:blank document.

This condition exists in order to handle some sloppiness with respect to the
principals on initial about:blank documents. Chrome callers sometimes parent
chrome windows (with XUL document) to content windows. But this parenting causes
us to push the cx of the content window during window creation, meaning that
the subsequent load of chrome://foo.xul blows away the old inner window and any
expandos on it. We can handle this case more precisely by skipping the cx push
for type="chrome" windows.

Furthermore, this was also necessary to prevent the inner window from being
blown away in the call to SetOpenerScriptPrincipal once nsWindowWatcher gets
the window back from the window creator (and after it's already told consumers
about the window via "domwindowcreated"). But we fixed this nastiness in the
previous patches.

So we can remove this case. By doing so, we can prevent inner windows from ever
changing origins, which is very important for compartment security invariants.

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 7 - Assert against switching origins on a compartment. v1

This is a very good thing to assert. \o/

Comment 19

[Bobby Holley (:bholley)]

Bouncing these reviews to jst since bz is out for a while. This stuff is pretty important security-wise to get onto aurora, so hopefully jst (and maybe mrbkap) can squeeze some time in here.

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 646202 [details] [diff] [review]
Part 1 - Factor nsWindowWatcher call to SetOpenerScriptPrincipal into a method on nsGlobalWindow. v1

- In dom/base/nsPIDOMWindow.h:

+  virtual void SetInitialPrincipalToSubject(nsIDocShellTreeItem* aItem,
+                                            nsIDOMWindow* aParent) = 0;

Bump the IID in this interface...

r=jst

Comment 21

[Bobby Holley (:bholley)]

Hell yeah - Thanks jst!

Comment 22

[Bobby Holley (:bholley)]

Did the iid bump and pushed this to try, since it's been a while:
https://tbpl.mozilla.org/?tree=Try&rev=e5807aad14d2

Comment 23

[Bobby Holley (:bholley)]

Looks green.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d84551808abb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/407f19deb14c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d49e7c0d762c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd07fd5147d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/35334e820632
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d4d36e3b948f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/036eb8c2a08a

Comment 24

[Bobby Holley (:bholley)]

Comment on attachment 646209 [details] [diff] [review]
Part 7 - Assert against switching origins on a compartment. v1

Flagging this for m-a approval, implicitly along with bug 774607 and bug 771354 (I'm treating them as one landing).

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 754202
User impact if declined: security bugs
Testing completed (on m-c, etc.): green on try. Just landed to m-i.
Risk to taking this patch (and alternatives if risky): Medium risk. Backing out bug 754202 would be higher risk though. 
String or UUID changes made by this patch: IID rev for nsPIDOMWindow.

Comment 25

[Bobby Holley (:bholley)]

Attachment: aurora patches

jst is going to land these to m-a tomorrow. Attaching combined aurora patches.

Comment 26

[Alex Keybl [:akeybl]]

(In reply to Bobby Holley (:bholley) (on vacation Aug 24 - Sept 3) from comment #24)
> Risk to taking this patch (and alternatives if risky): Medium risk. Backing
> out bug 754202 would be higher risk though. 
> String or UUID changes made by this patch: IID rev for nsPIDOMWindow.

Bobby and I discussed this - nsPIDOMWindow should be mostly used internally, and making this IID change now rather than later will be better for add-ons in the long run. As Bobby noted, this is approved for landing tomorrow so that we get at least a  day of bake time on Nightly.

Comment 27

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/deeadcce3f64 - it's not yet clear what with the way we pile bustage on bustage on bustage, but it looks like this was hanging every flavor of Windows build as it entered obj-firefox/widget/tests in make check.

Comment 28

[Phil Ringnalda (:philor)]

And something in that backout, most likely this, did cure the hangs.

Comment 29

[Johnny Stenback (:jst)]

This just landed on aurora cause it's something we do need there. We'll need to keep an eye on this and if the hangs come back, we need to either attempt to fix this there, or back this out. Unfortunately bholley is out on vacation for the next week.

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/e78456d2f05c
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/26c9db582c13
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/a27800787e7f
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/16454afdf3aa
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/047997b71f09
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ed313f650bbf
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/f304efa9667a

Comment 30

[Phil Ringnalda (:philor)]

Only one Windows build has gotten through its timeout and reported that it hung, but because they are all overdue, I'm sure they'll all hang just like they did on inbound. As a bonus, every debug mochitest-4 has blown up with a "Assertion failure: sameOrigin, at e:/builds/moz2_slave/m-aurora-w32-dbg/build/dom/base/nsGlobalWindow.cpp:1902".

Aurora's closed.

Comment 31

[Matt Brubeck (:mbrubeck)]

Backed out on Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/1f17cdc4fe1b

Comment 32

[Alex Keybl [:akeybl]]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #29)
> This just landed on aurora cause it's something we do need there. We'll need
> to keep an eye on this and if the hangs come back, we need to either attempt
> to fix this there, or back this out. Unfortunately bholley is out on
> vacation for the next week.

Is there anybody else that can check if there is a simple fix? My concern is that  with bholley out for the next week, a fix won't go in until Beta 3.

It'd also be good to understand if the medium risk evaluation was around the IID change, or something more. Perhaps backing out bug 754202 prior to Beta 1 (going to build Monday) should be on the table again.

Finally, CC'ing jorgev to see how he feels about making an nsPIDOMWindow IID change that late in the cycle (b3).

Comment 33

[Gabor Krizsanits (INACTIVE)]

     // Inner windows are only reused for same-origin principals, but the principals
     // don't necessarily match exactly. Update the principal on the compartment to
     // match the new document.
     // NB: We don't just call currentInner->RefreshCompartmentPrincipals() here
     // because we haven't yet set its mDoc to aDocument.
-    JS_SetCompartmentPrincipals(js::GetObjectCompartment(currentInner->mJSObject),
+    JSCompartment *compartment = js::GetObjectCompartment(currentInner->mJSObject);
+#ifdef DEBUG
+    bool sameOrigin = false;
+    nsIPrincipal *existing =
+      nsJSPrincipals::get(JS_GetCompartmentPrincipals(compartment));
+    aDocument->NodePrincipal()->Equals(existing, &sameOrigin);
+    MOZ_ASSERT(sameOrigin);

This assertion is not in sync with the comment. Shouldn't we assert for  nsScriptSecurityManager::CheckSameOriginPrincipal(existing,aDocument->NodePrincipal()) ?

Comment 34

[Gabor Krizsanits (INACTIVE)]

By the way, does anyone know which tests tend to hang (on windows)?

Comment 35

[Matt Brubeck (:mbrubeck)]

Here's one of the logs with the hang.  As noted in comment 27, it seems to hang when starting 'make check' in widget/tests, before any test output has been printed:

https://tbpl.mozilla.org/php/getParsedLog.php?id=14688923&tree=Mozilla-Aurora
make[2]: Leaving directory `/e/builds/moz2_slave/m-aurora-w32/build/obj-firefox/widget/windows'
make -C tests check
make[2]: Entering directory `/e/builds/moz2_slave/m-aurora-w32/build/obj-firefox/widget/tests'
command timed out: 300 seconds without output, attempting to kill
SIGKILL failed to kill process

Comment 36

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Alex Keybl [:akeybl] from comment #32)
> Finally, CC'ing jorgev to see how he feels about making an nsPIDOMWindow IID
> change that late in the cycle (b3).

It's a potential problem with binary add-ons, for sure. If we can get this in earlier in the cycle, that would be ideal. b3 is not terrible, considering there are security concerns. We would probably need to do some additional messaging when the patch is out (telling binary add-on devs they need to rebuild, without many specifics).

Comment 37

[Bobby Holley (:bholley)]

Attachment: mc-patches

Attaching a bundle of rebased m-c patches. Gabor is going to try to reproduce the windows hangs.

Comment 38

[Gabor Krizsanits (INACTIVE)]

In nsGlobalWindow::SetNewDocument if there is a currentInner (which is null pre-patch and set post-patch) the currentInner->FreeInnerObject() is called that nulls the mListenerManager of the window, and the load even fails to be deliver to the test object of TestAppShellSteadyState and it hangs.

Comment 39

[Gabor Krizsanits (INACTIVE)]

I was too tired to continue yesterday but here are some more details:

There are two calls to SetNewDocument post-patch but only one in the pre-patch case. Here are the two call stack, the first one that sets inner and cause the side effect in the second call happens only in the patched version.

First call stack of SetNewDocument, that is from the new SetInitialPrincipalToSubject call: (sets inner window)

>	xul.dll!nsGlobalWindow::SetNewDocument(nsIDocument * aDocument=0x0662b490, nsISupports * aState=0x00000000, bool aForceReuseInnerWindow=false)  Line 1959	C++
 	xul.dll!DocumentViewerImpl::InitInternal(nsIWidget * aParentWidget=0x0412f408, nsISupports * aState=0x00000000, const nsIntRect & aBounds={...}, bool aDoCreation=true, bool aNeedMakeCX=true, bool aForceSetNewDocument=true)  Line 927	C++
 	xul.dll!DocumentViewerImpl::Init(nsIWidget * aParentWidget=0x0412f408, const nsIntRect & aBounds={...})  Line 677	C++
 	xul.dll!nsDocShell::SetupNewViewer(nsIContentViewer * aNewViewer=0x06637160)  Line 8002 + 0x3b bytes	C++
 	xul.dll!nsDocShell::Embed(nsIContentViewer * aContentViewer=0x06637160, const char * aCommand=0x129cc7a1, nsISupports * aExtraInfo=0x00000000)  Line 6060 + 0x1c bytes	C++
 	xul.dll!nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal * aPrincipal=0x041d8900, nsIURI * aBaseURI=0x00000000, bool aTryToSaveOldPresentation=true)  Line 6795	C++
 	xul.dll!nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal * aPrincipal=0x041d8900)  Line 6812	C++
 	xul.dll!nsGlobalWindow::SetInitialPrincipalToSubject()  Line 1544	C++
 	xul.dll!nsAppShellService::RegisterTopLevelWindow(nsIXULWindow * aWindow=0x04131700)  Line 461	C++
 	xul.dll!nsAppShellService::CreateTopLevelWindow(nsIXULWindow * aParent=0x00000000, nsIURI * aUrl=0x04131668, unsigned int aChromeMask=1, int aInitialWidth=100, int aInitialHeight=100, nsIXULWindow * * aResult=0x0034d9e4)  Line 153	C++
 	TestAppShellSteadyState.exe!Test4Internal(nsIAppShell * aAppShell=0x0412e568)  Line 411 + 0x44 bytes	C++
 	TestAppShellSteadyState.exe!Test4(nsIAppShell * aAppShell=0x0412e568)  Line 443 + 0x9 bytes	C++
 	TestAppShellSteadyState.exe!NextTestRunnable::Run()  Line 467 + 0x2c bytes	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=false, bool * result=0x0034dacf)  Line 624 + 0x19 bytes	C++
 	xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x03cd0c88, bool mayWait=false)  Line 220 + 0x17 bytes	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate=0x0337bc20)  Line 82 + 0xe bytes	C++
 	xul.dll!MessageLoop::RunInternal()  Line 209	C++
 	xul.dll!MessageLoop::RunHandler()  Line 202	C++
 	xul.dll!MessageLoop::Run()  Line 176	C++
 	xul.dll!nsBaseAppShell::Run()  Line 165	C++
 	xul.dll!nsAppShell::Run()  Line 232 + 0x9 bytes	C++
 	TestAppShellSteadyState.exe!main(int argc=1, char * * argv=0x0337a7d0)  Line 494 + 0x19 bytes	C++
 	TestAppShellSteadyState.exe!__tmainCRTStartup()  Line 555 + 0x19 bytes	C
 	TestAppShellSteadyState.exe!mainCRTStartup()  Line 371	C
 	kernel32.dll!75303677() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	
 	ntdll.dll!77739f42() 	
 	ntdll.dll!77739f15() 	

First call stack of SetNewDocument, that is called later (this call occures pre and post patch as well) and calls currentInner->FreeInnerObjects();
that resets the mListenerManager

>	xul.dll!nsGlobalWindow::SetNewDocument(nsIDocument * aDocument=0x06799040, nsISupports * aState=0x00000000, bool aForceReuseInnerWindow=false)  Line 1955	C++
 	xul.dll!DocumentViewerImpl::InitInternal(nsIWidget * aParentWidget=0x0412f408, nsISupports * aState=0x00000000, const nsIntRect & aBounds={...}, bool aDoCreation=true, bool aNeedMakeCX=true, bool aForceSetNewDocument=true)  Line 927	C++
 	xul.dll!DocumentViewerImpl::Init(nsIWidget * aParentWidget=0x0412f408, const nsIntRect & aBounds={...})  Line 677	C++
 	xul.dll!nsDocShell::SetupNewViewer(nsIContentViewer * aNewViewer=0x067ba9a0)  Line 8002 + 0x3b bytes	C++
 	xul.dll!nsDocShell::Embed(nsIContentViewer * aContentViewer=0x067ba9a0, const char * aCommand=0x129cc7a2, nsISupports * aExtraInfo=0x00000000)  Line 6060 + 0x1c bytes	C++
 	xul.dll!nsDocShell::CreateContentViewer(const char * aContentType=0x042126e0, nsIRequest * request=0x0661a648, nsIStreamListener * * aContentHandler=0x0661a980)  Line 7789 + 0x28 bytes	C++
 	xul.dll!nsDSURIContentListener::DoContent(const char * aContentType=0x042126e0, bool aIsContentPreferred=false, nsIRequest * request=0x0661a648, nsIStreamListener * * aContentHandler=0x0661a980, bool * aAbortProcess=0x0034d7ab)  Line 120 + 0x20 bytes	C++
 	xul.dll!nsDocumentOpenInfo::TryContentListener(nsIURIContentListener * aListener=0x04134930, nsIChannel * aChannel=0x0661a648)  Line 678 + 0x42 bytes	C++
 	xul.dll!nsDocumentOpenInfo::DispatchContent(nsIRequest * request=0x0661a648, nsISupports * aCtxt=0x00000000)  Line 375 + 0x35 bytes	C++
 	xul.dll!nsDocumentOpenInfo::OnStartRequest(nsIRequest * request=0x0661a648, nsISupports * aCtxt=0x00000000)  Line 263 + 0x10 bytes	C++
 	xul.dll!nsBaseChannel::OnStartRequest(nsIRequest * request=0x0661aac0, nsISupports * ctxt=0x00000000)  Line 704 + 0x46 bytes	C++
 	xul.dll!nsInputStreamPump::OnStateStart()  Line 417 + 0x2c bytes	C++
 	xul.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream=0x0661b920)  Line 368 + 0xb bytes	C++
 	xul.dll!nsInputStreamReadyEvent::Run()  Line 83	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=false, bool * result=0x0034dacf)  Line 624 + 0x19 bytes	C++
 	xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x03cd0c88, bool mayWait=false)  Line 220 + 0x17 bytes	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate=0x0337bc20)  Line 82 + 0xe bytes	C++
 	xul.dll!MessageLoop::RunInternal()  Line 209	C++
 	xul.dll!MessageLoop::RunHandler()  Line 202	C++
 	xul.dll!MessageLoop::Run()  Line 176	C++
 	xul.dll!nsBaseAppShell::Run()  Line 165	C++
 	xul.dll!nsAppShell::Run()  Line 232 + 0x9 bytes	C++
 	TestAppShellSteadyState.exe!main(int argc=1, char * * argv=0x0337a7d0)  Line 494 + 0x19 bytes	C++
 	TestAppShellSteadyState.exe!__tmainCRTStartup()  Line 555 + 0x19 bytes	C
 	TestAppShellSteadyState.exe!mainCRTStartup()  Line 371	C
 	kernel32.dll!75303677() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	
 	ntdll.dll!77739f42() 	
 	ntdll.dll!77739f15()

Comment 40

[Bobby Holley (:bholley)]

Ok, so here's what's going on.

In Test4Internal of TestAppShellSteadyState.cpp ( http://mxr.mozilla.org/mozilla-central/source/widget/tests/TestAppShellSteadyState.cpp#387 ), we do a direct call to CreateTopLevelWindow with about:blank as the URI.

The call to CreateTopLevelWindow calls into RegisterTopLevelWindow, which, post-patch, now calls SetInitialPrincipalToSubject (this formerly happened only in the window watcher code, which never got triggered by this standalone test). This replaces the initial about:blank document with another about:blank document with system principal.

We then queue up an asynchronous load of the URI "about:blank", and proceed to add event listeners to the window, and return. But when the async load happens, the new document ends up with the principal "about:blank", which blows away the old inner window and all associated event listeners. So the listener never gets called.

Comment 41

[Bobby Holley (:bholley)]

Attachment: Part 8 - Fix TestAppShellSteadyState to load a system principal URI so that we don't discard the inner window. r=bent

Talked this fix over with bent. This appears to be the path of least resistance.

Comment 42

[Bobby Holley (:bholley)]

Relanded to m-i with the fix for the hanging windows make check:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=cfc884e6e414

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5cb753a50e22
https://hg.mozilla.org/mozilla-central/rev/4885a6b70c02
https://hg.mozilla.org/mozilla-central/rev/a22a0ad64bb6
https://hg.mozilla.org/mozilla-central/rev/783bd41565ec
https://hg.mozilla.org/mozilla-central/rev/32ecf0cec2c3
https://hg.mozilla.org/mozilla-central/rev/b33228bc231a
https://hg.mozilla.org/mozilla-central/rev/3ae9e26467f6
https://hg.mozilla.org/mozilla-central/rev/a0e42f6c2359

Comment 44

[Bobby Holley (:bholley)]

Pushed to m-a:

https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=b8ed43695721

Comment 45

[Bobby Holley (:bholley)]

Attachment: Part 1 sans IID rev

I've put together a patch stack that doesn't rev IIDs. Flagging jst for review on the parts with significant updates.

Comment 46

[Bobby Holley (:bholley)]

Attachment: Part 3 sans IID rev

Comment 47

[Bobby Holley (:bholley)]

Attachment: Part 4 sans IID rev

Comment 48

[Bobby Holley (:bholley)]

So I've been tearing my hair out trying to figure out why we got the M-4 assertion on aurora but I can't reproduce it locally (given that we don't have great try coverage on branches). However, I just realized that Johnny only landed the patches from this bug, not the entire zip I posted. The patches in bug 774607 and bug 771354 are (unfortunately) inextricably tied to this one. In particular, that assertion will fire without the patch in bug 771354.

Landed the whole shebang to beta:
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=6fbaf3aed7a0

Comment 49

[:Gavin Sharp [email: [e-mail]]]

Shouldn't this be marked FIXED, given comment 43?

Comment 50

[Al Billings [:abillings - ex-MoCo]]

This is a regression off of bug 754202 (based on comment 24), which landed on 6/29, if I read comments correctly. Does this mean that Firefox 15 was unaffected by this?

Comment 51

[Bobby Holley (:bholley)]

(In reply to Al Billings [:abillings] from comment #50)
> This is a regression off of bug 754202 (based on comment 24), which landed
> on 6/29, if I read comments correctly. Does this mean that Firefox 15 was
> unaffected by this?

Correct.

Comment 53

[Bobby Holley (:bholley)]

These patches include an assertion in the code that will trigger if we ever get this wrong.

Comment 54

[Matt Wobensmith [:mwobensmith][:matt:]]

Based on Bobby's comment above, test failures would be observed as asserts on existing tests. Checkins landed on branches 16, 17, 18, so from a QA standpoint, I consider this verified.