769254 - Clicking a target=_blank link inside <iframe mozbrowser> crashes Gecko, should pass opened URL to mozbrowseropenwindow event

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Justin Lebar (not reading bugmail)]

See https://github.com/mozilla-b2g/gaia/issues/1976.  Ouch.

Comment 1

[Justin Lebar (not reading bugmail)]

So the crash is easy enough to fix.

But there's a bigger problem.  We're crashing because the window-opening methods are getting a null URI, because that's what docshell gives us.

But when someone clicks a _blank link, my plan was that we'd fire a mozbrowseropenwindow event (possibly indicating that it was triggered from a link and not a window.open) and we'd handle things there.  In the case of _blank, the intent is for Gaia to open the browser app via a web activity.  But of course it can only do that if it gets the real URL, which isn't not getting atm!

Comment 2

[Jason Smith [:jsmith]]

Dietrich, Chris - Looks like a basecamp blocker, right?

Comment 3

[Justin Lebar (not reading bugmail)]

The crash is an obvious blocker.  I think the deeper issue in comment 1 is also a blocker, unless we can figure out some other way to handle _blank links.

Comment 4

[Justin Lebar (not reading bugmail)]

We'll almost certainly have to be smarter than this, but here's a push where I just send the URL along in docshell for all open-window calls, where we used to send the empty string.  https://tbpl.mozilla.org/?tree=Try&rev=5a728b64915f

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v1: Fix crashes in BrowserElementParent.cpp, TabChild.cpp on null URI to window-opening code.

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Part 2: Pass in the URL for target=_blank links.

Comment 7

[Justin Lebar (not reading bugmail)]

Attachment: Part 3: Rename nsWindowWatcher::OpenWindowJSInternal --> OpenWindowInternal.

Comment 8

[Justin Lebar (not reading bugmail)]

Attachment: Part 4: Tests for clicking _blank link from <iframe mozbrowser>.

Comment 9

[Justin Lebar (not reading bugmail)]

Comment on attachment 638343 [details] [diff] [review]
Part 1, v1: Fix crashes in BrowserElementParent.cpp, TabChild.cpp on null URI to window-opening code.

On second thought, this is essentially a docshell change, so a docshell peer should probably look at this.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 638343 [details] [diff] [review]
Part 1, v1: Fix crashes in BrowserElementParent.cpp, TabChild.cpp on null URI to window-opening code.

r=me, but you should probably document in BrowserElementParent.h that the passed-in value is an _absolute_ URI and empty string means to not load anything, right?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 638344 [details] [diff] [review]
Part 2: Pass in the URL for target=_blank links.

>Bug 769254 - Part 2: Pass in the URL for target=_blank links.

This could use a more clear description of the change...

>+++ b/dom/base/nsGlobalWindow.h
>+  // XXX comment

It's commented on nsPIDOMWindow, right?

You need to change the nsPIDOMWindow IID.

Please document the changed behavior for nsIWindowProvider (this part should perhaps be dev-doc-needed) and check that our existing impls all handle it fine.

r=me with that.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 638345 [details] [diff] [review]
Part 3: Rename nsWindowWatcher::OpenWindowJSInternal --> OpenWindowInternal.

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 638346 [details] [diff] [review]
Part 4: Tests for clicking _blank link from <iframe mozbrowser>.

r=me

Comment 15

[Justin Lebar (not reading bugmail)]

> nsIWindowProvider::provideWindow()
>
> * @param aURI The URI to be loaded in the new window.  The nsIWindowProvider
> *             implementation MUST NOT load this URI in the window it
> *             returns.  This URI is provided solely to help the
> *             nsIWindowProvider implementation make decisions; the caller
> *             will handle loading the URI in the window returned if
> *             provideWindow returns a window.  Note that the URI may be null
> *             if the load cannot be represented by a single URI (e.g. if
> *             the load has extra load flags, POST data, etc).

Aside from the weasel-y "/may/ be null for POST", this seems pretty accurate to me as-is.  Am I forgetting something here?

Comment 16

[Justin Lebar (not reading bugmail)]

Comment on attachment 638344 [details] [diff] [review]
Part 2: Pass in the URL for target=_blank links.

So, this breaks modal dialogs, because I didn't handle the args array properly.  Patch forthcoming, but maybe I'll wait for try this time before asking for review.

Comment 17

[Justin Lebar (not reading bugmail)]

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

Comment 18

[Mozilla RelEng Bot]

Try run for 4f0cd49d3031 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4f0cd49d3031
Results (out of 17 total builds):
    failure: 17
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4f0cd49d3031

Comment 19

[Boris Zbarsky [:bzbarsky]]

Well, the comment currently claims that the URI may be null for POST... except it won't be.

Better to just say that:

1)  The URI may be null.
2)  Even when not null it may not be enough to do the load correctly (e.g. ....)

Comment 20

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v2

My previous part 2 didn't handle modal dialog args properly.  This fixes it and looks good on try, modulo my intermittent orange issues.  Interdiff in a moment.

Comment 21

[Justin Lebar (not reading bugmail)]

Attachment: Interdiff part 2, v1 -> v2

Comment 22

[Justin Lebar (not reading bugmail)]

Comment on attachment 639923 [details] [diff] [review]
Interdiff part 2, v1 -> v2

> +            /*rv = win->Open(EmptyString(), // URL to load
> +                           name,          // window name
> +                           EmptyString(), // Features
> +                           getter_AddRefs(newWin));*/

I fixed this; it's a stale interdiff.  :-/

The args parameter to openWindow is pretty poorly documented.  We can improve it if you want, but to be honest, I'm not entirely sure what it is for!  :)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 639922 [details] [diff] [review]
Part 2, v2

r=me.  Thanks for the interdiff!  That made life much simpler.

Comment 24

[Matt Brubeck (:mbrubeck)]

Sorry, I had to back this out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b33564258377

because of leaks in mochitest-plain-3 on Windows:
https://tbpl.mozilla.org/php/getParsedLog.php?id=13441758&tree=Mozilla-Inbound

Comment 25

[Justin Lebar (not reading bugmail)]

This is going to be fun to debug without working tryserver.  :(

Comment 26

[Justin Lebar (not reading bugmail)]

> leaked 8 instances of nsXULWindow with size 144 bytes each (1152 bytes total)

Yikes.  M3 doesn't even run the browser-element tests.  I wonder what's going on here.

Comment 27

[Justin Lebar (not reading bugmail)]

Maybe I messed up the args code?  That's my best guess at this point.

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

Comment 28

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbdb3104c9e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2fe54ae00a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d378362cbe01
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe623d60bea1

Comment 29

[Matt Brubeck (:mbrubeck)]

Sorry, I backed this out because it caused frequent (~50%) timeouts in Windows debug mochitests-3 runs.  See bug 677841 comment 46 for some more discussion.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a26e751bfb54

Comment 30

[Ed Morley [:emorley]]

(In reply to Matt Brubeck (:mbrubeck) from comment #29)
> Sorry, I backed this out because it caused frequent (~50%) timeouts in
> Windows debug mochitests-3 runs.  See bug 677841 comment 46 for some more
> discussion.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a26e751bfb54

Just to confirm: this bug was indeed the cause; prior to the backout there was about a 40-50% failure rate - post backout we've had 15 consecutive green runs.

Comment 32

[Justin Lebar (not reading bugmail)]

Try push with the resize_window test disabled:

https://tbpl.mozilla.org/?tree=Try&rev=7b93b1737715

Comment 33

[Justin Lebar (not reading bugmail)]

Just an update: I haven't forgotten about this bug; it's just progressing very slowly because I've only managed to reproduce the latest random oranges on tryserver, and my last push took more than 12 hours to get me the test results.

I'm going to push the crashfix (part 1) and keep working on the rest of this bug, so people stop getting bitten by the worst part of the bug.

Comment 34

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7c1adbb8182

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e7c1adbb8182

Comment 36

[Justin Lebar (not reading bugmail)]

Okay, so I've figured out the/a problem.

nsGlobalWindow::ShowmModalDialog calls nsGlobalWindow::OpenInternal and passes

  aDialog = false,
  aCalledNoScript = false.

Both of these are blatant lies, afaict.  The old code recovered from the aDialog = false lie by setting aDialog to |args.length > 0| (in nsWindowWatcher::OpenWindow), but this patch removed this check.

Comment 37

[Justin Lebar (not reading bugmail)]

FWIW git blame shows that the args were set this way when ShowModalDialog was originally written, back in bug 194404.  :)

Comment 38

[Justin Lebar (not reading bugmail)]

We need aCalledNoScript = false, or otherwise the window watcher won't respect -moz-internal-modal=1, so the window won't be modal.

If we force aDialog = true in ShowModalDialog, I bet that would cause the window opened by showModalDialog("http://foo.com") to have window.dialogArguments == null, in violation of the spec [1], which says it should be undefined.

So I guess we have to continue doing basically what we're doing now.

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage//timers.html#dom-showmodaldialog (currently step 10).

Comment 39

[Justin Lebar (not reading bugmail)]

Famous last words, but this might work: https://tbpl.mozilla.org/?tree=Try&rev=1ac7960758d2

Comment 40

[Justin Lebar (not reading bugmail)]

Attachment: Part 2a: Fix aDialog parameter in nsWindowWatcher.

I tried fixing this by fixing the lies that ShowModalDialog tells, but it was
complicated.  I'd like to land this and worry about prettifying this code in
bug 779939, if that's OK.

This is finally green on tryserver: https://tbpl.mozilla.org/?tree=Try&rev=ad553d76a50d

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 648398 [details] [diff] [review]
Part 2a: Fix aDialog parameter in nsWindowWatcher.

r=me

Comment 42

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7f3d9cf9b3a1 (the commit message only mentions backing out dcb9d2f694eb, but I did back out 22d637d39566 as well) - for the most part, when we did manage to get around to running mochitest-3 on Windows today, starting with you it timed out in test_resize_move_windows.html.

Comment 43

[Justin Lebar (not reading bugmail)]

This is so nuts.

Comment 44

[Justin Lebar (not reading bugmail)]

Ah, have another look at the diff:

> +  PRUint32 argc = 0;
> +  if (argv) {
> +    argv->GetLength(&argc);
> +  }
> +
> +  bool dialog = aDialog;
> +  if (!aCalledFromScript) {
> +    dialog = argc > 0;
> +  }
> +
>    return OpenWindowInternal(aParent, aUrl, aName, aFeatures,
>                              aCalledFromScript, aDialog,
>                              aNavigate, argv, _retval);

I pass aDialog to OpenWindowInternal, not dialog.  I have no idea how I did this...

Comment 45

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=054c2a61eec8

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b9d4cc7af705
https://hg.mozilla.org/mozilla-central/rev/5bda15ef0b4c