Following up on https://bugzilla.mozilla.org/show_bug.cgi?id=764414#c6 Looks like because of this the window is not seen as having been opened by a script which prevents window.close.

ReadyOpenedDocShellItem is being called, but, in the in-process case, windowIsNew is false, because of a bug in the window.open patch. In the out-of-process case, that part looks correct, but then we crash later on in my test. :(

In the in-process case, when we send the correct windowIsNew value, we get an assertion in nsGlobalWindow. nsWindowWatcher::OpenWindowJSInternal calls nsGlobalWindow::SetOwnerScriptPrincipal, which asserts that mDoc->NodePrincipal()->GetURI is about:blank. mDoc->mDocumentURI is indeed about:blank, but at this point, its principal is the parent's URI. Parent, not opener -- that is, if I have foo.html <iframe mozbrowser src='popup_opener.html'> and popup_opener.html calls window.open(), then the node principal of the popup document, at this point, anyway, will be foo.html. bz, I presume we're seeing this because nsGlobalWindow expects this window is a true popup, when in fact it's an iframe. And I presume this old principal gets overwritten soon enough. Should we just change the assertion to check the document's URI instead of its principal? Or is something deeper going on here?

> I presume we're seeing this because nsGlobalWindow expects this window is a true popup Well, it's expecting that the new thing doesn't have a nontrivial principal yet. We should probably cut off principal inheritance into subframes at mozbrowser just like we do at the chrome/content boundary, right? It's not exactly expected that if you add an <iframe mozbrowser src="about:blank"> it'll get its parent's principal, I bet.

In the OOP-enabled case, we assert because, it seems, calling window.close() does not cancel the network load. But I bet if I closed the window *and* removed the "popup" iframe, we wouldn't assert. If so, this probably isn't a big deal.

I don't know why I didn't think of this until now, but if the plan is to run all mozbrowsers out of process, then we shouldn't really care about getting the right principal in the in-process case. It's nice if it kind of works, of course, since we're still in the period of transition away from in-process.

bz, I thought the frameloader change we discussed (if it's mozbrowser, set inherit owner on the loadinfo in ReallyStartLoadingInternal) was for the following case: var iframe = document.createElement('iframe'); iframe.mozbrowser = true; document.body.appendChild(iframe); if (iframe.contentWindow.location == 'foo') { ... } The idea was that the last line should throw, because the iframe won't have the same principal as the embedder, right? Changing only nsDocShell::GetInheritedPrincipal gets us this behavior. I'll post a patch with a test in a minute.

cjones now tells me that we may not have nested content processes, in which case we'll have both remote and non-remote mozbrowsers. I guess I'll try to fix this properly.

> The idea was that the last line should throw, because the iframe won't have the same > principal as the embedder, right? Well, it's not clear whether the embedder should subsume it or not.... > Changing only nsDocShell::GetInheritedPrincipal gets us this behavior. Sure. The frameloader change I talked about would matter for purposes of what happens if you do the contentWindow.location bit off a timeout in the code in comment 8.

> The frameloader change I talked about would matter for purposes of what happens if you do the > contentWindow.location bit off a timeout in the code in comment 8. Okay, got that. I gather that: If we touch contentWindow.location before the timeout, then that calls EnsureContentViewer() immediately. Whereas if we touch contentWindow.location off a timeout, then that's after the frameloader has had a chance to load about:blank into the iframe. Is that more correct than wrong? But now, in FrameLoader::LoadURI for mozbrowser, what's the difference between 1) replacing the SetOwner() call with InheritOwner(true), and 2) simply not calling SetOwner()? Both pass the test with the timeout. With (1), it seems that the owner ends up being the current document. Whereas with (2), we "we pass a null owner into the channel, and an owner will be created later from the channel's internal data." But in this case, we're loading about:blank, so it seems to me that these two paths may be equivalent: With (1), we call GetInheritedPrincipal(aConsiderCurrentDocument = true). This calls EnsureContentViewer, which calls GetInheritedPrincipal(false), which returns null. EnsureContentViewer creates about:blank with null principal. Now our original GetInheritedPrincipal call gets the principal off our current document, which is about:blank with the null principal. Whereas, with (2), well...I'm not sure. There's no channel, right? But it seems there's nowhere for it to get a principal that's not null.

> Is that more correct than wrong? It's pretty much spot-on, in the current setup. (Henri's work to make about:blank sync might affect the behavior, if he ever gets it to pass tests.) > what's the difference between For the initial about:blank, I think you're right: there is no difference. The question is what should happen if the browser app sets the src attribute of a mozbrowser to "javascript:something". Should that run with the principals of the currently loaded page, or null principals, or the browser app's principals? For the system principal in Firefox, we force that to run with the principals of the currently loaded page. What behavior do you want in your case? (Note also that there's the question of what should happen on window.location sets to javascript: URIs, which take a different codepath and would, at the moment, run with the principal of the browser app).

> The question is what should happen if the browser app sets the src attribute of a mozbrowser to > "javascript:something". Should that run with the principals of the currently loaded page, or null > principals, or the browser app's principals? Certainly not the browser app's principal, because that doesn't work with OOP. And running the javascript: URL with the principal of the page would give the embedder permission to run arbitrary JS on the content page, right? If so, sounds like we should run with the null principal.

> running the javascript: URL with the principal of the page would give the embedder > permission to run arbitrary JS on the content page Yes. That is rather the point of bookmarklets, say.

> Yes. That is rather the point of bookmarklets, say. Indeed. I don't think bookmarklets will work in the browser app as-is, so unless/until we want to support them, I think we might as well do the safest thing.

This fixes the assertions manifested in parts 1 and 2. Famous last words: I *think* this is correct.

Comment on attachment 633228 [details] [diff] [review] Tests, v1 Let's say r=me

Comment on attachment 633229 [details] [diff] [review] Fix for OOP disabled. (Still asserts.) r=me

Comment on attachment 633230 [details] [diff] [review] Fix for OOP enabled. (Also asserts.) Why is there no event target? I assume that's the important part of this patch, yes? ;)

Comment on attachment 634978 [details] [diff] [review] Part 3: Set the initial principal of <iframe mozbrowser> correctly. r=me

(In reply to Boris Zbarsky (:bz) from comment #19) > Why is there no event target? I assume that's the important part of this > patch, yes? ;) You mean, figuring things out instead of blindly adding null-checks until it no longer crashes? :) I went back to get the stack trace of the crash, and I can no longer reproduce it (even without the other non-test patches in this bug). So...I guess we can just drop the patch. Next time I'll paste the stacktrace into the bug!

https://hg.mozilla.org/integration/mozilla-inbound/rev/d220195a8b95 https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb8d7a3852f https://hg.mozilla.org/integration/mozilla-inbound/rev/fed89ccfe0c2

Backed out for mochitest 2 failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=fed89ccfe0c2 https://hg.mozilla.org/integration/mozilla-inbound/rev/db1a0ca3bc67

Oh, duh, this bug depends on bug 765075. That's why I didn't push it last night!

Sorry backed out since this is failing pretty much every push in some way or another: https://tbpl.mozilla.org/php/getParsedLog.php?id=12884012&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12884081&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12884750&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12885725&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12885250&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12886456&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12889153&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12889275&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12889284&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12890073&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12889984&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12890596&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12890908&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12890642&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12892910&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12893133&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=12893224&tree=Mozilla-Inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/1da4a83db7b2

My guess is that the culprit here is bug 765075, since when I landed this bug earlier, without that one, we didn't see a mass of intermittent orange, and also because that bug touches loading of BrowserElementChild.js, which could result in this kind of widespread orange.

I now think that this is the problem, not bug 765075. One failure which happens frequently in the try runs is that accessing frameElement.ownerDocument.defaultView throws a "can't access dead object" error. (frameElement is the <iframe mozbrowser> element.) I'm also getting > [Parent 359] WARNING: Not same origin error!: file ../../../dom/base/nsJSEnvironment.cpp, line 347 around here. I don't have a stacktrace, but it's likely this is happening from the same access. I'm pretty confused by this. It appears this was caused by the iframe principal changes in this bug. But why does the iframe's principal matter at all when accessing its owner document?

(In reply to Justin Lebar [:jlebar] from comment #29) > I'm pretty confused by this. It appears this was caused by the iframe > principal changes in this bug. But why does the iframe's principal matter > at all when accessing its owner document? See bug 695480 - "chrome" code can no longer hold on to "dead" content elements. I don't know the exact details definitions of "chrome" and "dead" that are being used, but presumably that's the issue here. Talk to khuey?

> I don't know the exact details definitions of "chrome" and "dead" that are being used, "chrome" means "chrome-to-content xpconnect wrappers", and "dead" means "part of a detached window" which means "the window doesn't have a docshell, which means "its iframe isn't part of a document." But it's still not clear to me how that affects things here. We're not accessing the iframe's contentWindow, but its ownerDocument, which should be /us/. And /we're/ not detached. I think. It's still a mystery to me how principal changes could cause this. Although I understand that if you even look at this code the wrong way, the tree starts going intermittent orange...

This principal change is a can of worms, but the other two patches here ("tests" and "fix for OOP disabled") are simple and helpful. I'm going to try to land those, close this bug, and deal with the principal business separately.

"fix for OOP disabled" is needed to make bug 771273 work properly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e47e74e3578 https://hg.mozilla.org/integration/mozilla-inbound/rev/82f24178d2cb I'll file a separate bug for the principal work.

Sorry, I had to back this out on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/c990f7d9ee49 because test_browserElement_oop_CloseFromOpener.html and test_browserElement_oop_GetScreenshot.html were failing frequently on Mac builds: https://tbpl.mozilla.org/php/getParsedLog.php?id=13296535&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=13297498&tree=Mozilla-Inbound

I've been having such bad luck with these tests...

Try run for 6e47bc0926f6 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=6e47bc0926f6 Results (out of 300 total builds): exception: 1 success: 256 warnings: 35 failure: 8 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-6e47bc0926f6

Try run for 486818f775ba is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=486818f775ba Results (out of 169 total builds): exception: 39 success: 117 warnings: 9 failure: 4 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-486818f775ba

https://hg.mozilla.org/mozilla-central/rev/1bef7c0e655b https://hg.mozilla.org/mozilla-central/rev/aa3ac9e05a56