Calling close() on a window opened by an <iframe mozbrowser> doesn't work (no effect in process, crash out of process)

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: etienne, Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug)

unspecified
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

(Whiteboard: [qa+])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Updated

6 years ago
Blocks: 742944
(Assignee)

Updated

6 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 1

6 years ago
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.  :(
(Assignee)

Updated

6 years ago
Summary: nsWindowWatcher::ReadyOpenedDocShellItem is never called during an <iframe mozbrowser> window.open → Calling close() on a window opened by an <iframe mozbrowser> doesn't work (no effect in process, crash out of process)
(Assignee)

Comment 2

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

Comment 5

6 years ago
Created attachment 633229 [details] [diff] [review]
Fix for OOP disabled.  (Still asserts.)
(Assignee)

Comment 6

6 years ago
Created attachment 633230 [details] [diff] [review]
Fix for OOP enabled.  (Also asserts.)

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.
(Assignee)

Comment 7

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

Comment 8

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

Comment 9

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

Updated

6 years ago
Depends on: 765075
> 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.
(Assignee)

Comment 11

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

Comment 13

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

Comment 15

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

Updated

6 years ago
blocking-basecamp: --- → ?

Updated

6 years ago
blocking-basecamp: ? → +
blocking-kilimanjaro: --- → +
(Assignee)

Updated

6 years ago
No longer blocks: 766481
(Assignee)

Updated

6 years ago
Blocks: 757182
(Assignee)

Comment 16

6 years ago
Created attachment 634978 [details] [diff] [review]
Part 3: Set the initial principal of <iframe mozbrowser> correctly.

This fixes the assertions manifested in parts 1 and 2.

Famous last words: I *think* this is correct.
Attachment #634978 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #633228 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #633229 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #633230 - Flags: review?(bzbarsky)
Comment on attachment 633228 [details] [diff] [review]
Tests, v1

Let's say r=me
Attachment #633228 - Flags: review?(bzbarsky) → review+
Comment on attachment 633229 [details] [diff] [review]
Fix for OOP disabled.  (Still asserts.)

r=me
Attachment #633229 - Flags: review?(bzbarsky) → review+
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
Attachment #634978 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 21

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

Updated

6 years ago
Attachment #633230 - Attachment is obsolete: true
Attachment #633230 - Flags: review?(bzbarsky)
(Assignee)

Comment 25

6 years ago
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

Updated

6 years ago
Depends on: 765613
(Assignee)

Comment 28

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

Comment 29

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

Comment 31

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

Comment 32

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

Comment 33

6 years ago
"fix for OOP disabled" is needed to make bug 771273 work properly.
Blocks: 771273
(Assignee)

Updated

6 years ago
Blocks: 771584
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
(Assignee)

Comment 36

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

Comment 37

6 years ago
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

Comment 38

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

Updated

6 years ago
Depends on: 772076

Updated

6 years ago
Whiteboard: [qa+]

Updated

6 years ago
Depends on: 819130

Updated

6 years ago
Depends on: 805185
No longer depends on: 805185
You need to log in before you can comment on or make changes to this bug.