Last Comment Bug 769254 - Clicking a target=_blank link inside <iframe mozbrowser> crashes Gecko, should pass opened URL to mozbrowseropenwindow event
: Clicking a target=_blank link inside <iframe mozbrowser> crashes Gecko, shoul...
Status: VERIFIED FIXED
[dev-doc-needed see comment 12]
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla17
Assigned To: Justin Lebar (not reading bugmail)
: Jason Smith [:jsmith]
: Andrew Overholt [:overholt]
Mentors:
: 764707 777164 (view as bug list)
Depends on: 677841 772076 775676 776801
Blocks: browser-api
  Show dependency treegraph
 
Reported: 2012-06-28 06:59 PDT by Justin Lebar (not reading bugmail)
Modified: 2013-04-04 13:53 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
Part 1, v1: Fix crashes in BrowserElementParent.cpp, TabChild.cpp on null URI to window-opening code. (3.79 KB, patch)
2012-07-02 07:05 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
justin.lebar+bug: checkin+
Details | Diff | Splinter Review
Part 2: Pass in the URL for target=_blank links. (27.67 KB, patch)
2012-07-02 07:05 PDT, Justin Lebar (not reading bugmail)
justin.lebar+bug: review-
Details | Diff | Splinter Review
Part 3: Rename nsWindowWatcher::OpenWindowJSInternal --> OpenWindowInternal. (7.08 KB, patch)
2012-07-02 07:05 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Splinter Review
Part 4: Tests for clicking _blank link from <iframe mozbrowser>. (5.18 KB, patch)
2012-07-02 07:05 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Splinter Review
Part 2, v2 (32.06 KB, patch)
2012-07-06 20:59 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Splinter Review
Interdiff part 2, v1 -> v2 (9.13 KB, patch)
2012-07-06 21:00 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Part 2a: Fix aDialog parameter in nsWindowWatcher. (1.53 KB, patch)
2012-08-02 11:17 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2012-06-28 06:59:40 PDT
See https://github.com/mozilla-b2g/gaia/issues/1976.  Ouch.
Comment 1 Justin Lebar (not reading bugmail) 2012-06-28 08:02:40 PDT
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] 2012-06-28 08:10:59 PDT
Dietrich, Chris - Looks like a basecamp blocker, right?
Comment 3 Justin Lebar (not reading bugmail) 2012-06-28 08:13:38 PDT
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) 2012-06-28 09:34:03 PDT
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) 2012-07-02 07:05:08 PDT
Created attachment 638343 [details] [diff] [review]
Part 1, v1: Fix crashes in BrowserElementParent.cpp, TabChild.cpp on null URI to window-opening code.
Comment 6 Justin Lebar (not reading bugmail) 2012-07-02 07:05:20 PDT
Created attachment 638344 [details] [diff] [review]
Part 2: Pass in the URL for target=_blank links.
Comment 7 Justin Lebar (not reading bugmail) 2012-07-02 07:05:32 PDT
Created attachment 638345 [details] [diff] [review]
Part 3: Rename nsWindowWatcher::OpenWindowJSInternal --> OpenWindowInternal.
Comment 8 Justin Lebar (not reading bugmail) 2012-07-02 07:05:42 PDT
Created attachment 638346 [details] [diff] [review]
Part 4: Tests for clicking _blank link from <iframe mozbrowser>.
Comment 9 Justin Lebar (not reading bugmail) 2012-07-02 07:07:04 PDT
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 10 Justin Lebar (not reading bugmail) 2012-07-02 10:31:54 PDT
*** Bug 764707 has been marked as a duplicate of this bug. ***
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-07-06 10:26:21 PDT
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 [:bz] (still a bit busy) 2012-07-06 10:49:54 PDT
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 [:bz] (still a bit busy) 2012-07-06 10:50:31 PDT
Comment on attachment 638345 [details] [diff] [review]
Part 3: Rename nsWindowWatcher::OpenWindowJSInternal --> OpenWindowInternal.

r=me
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2012-07-06 10:50:53 PDT
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) 2012-07-06 11:11:54 PDT
> 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) 2012-07-06 15:18:40 PDT
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) 2012-07-06 15:32:37 PDT
https://tbpl.mozilla.org/?tree=Try&rev=4f0cd49d3031
Comment 18 Mozilla RelEng Bot 2012-07-06 17:15:25 PDT
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/jlebar@mozilla.com-4f0cd49d3031
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2012-07-06 18:08:29 PDT
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) 2012-07-06 20:59:15 PDT
Created attachment 639922 [details] [diff] [review]
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) 2012-07-06 21:00:25 PDT
Created attachment 639923 [details] [diff] [review]
Interdiff part 2, v1 -> v2
Comment 22 Justin Lebar (not reading bugmail) 2012-07-06 21:03:40 PDT
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 [:bz] (still a bit busy) 2012-07-10 14:57:29 PDT
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) 2012-07-11 17:34:27 PDT
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) 2012-07-11 17:58:00 PDT
This is going to be fun to debug without working tryserver.  :(
Comment 26 Justin Lebar (not reading bugmail) 2012-07-12 06:59:23 PDT
> 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) 2012-07-16 10:20:49 PDT
Maybe I messed up the args code?  That's my best guess at this point.

https://tbpl.mozilla.org/?tree=Try&rev=baff55f8d945
Comment 29 Matt Brubeck (:mbrubeck) 2012-07-23 20:44:00 PDT
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] 2012-07-24 02:18:51 PDT
(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 31 Justin Lebar (not reading bugmail) 2012-07-25 06:58:13 PDT
*** Bug 777164 has been marked as a duplicate of this bug. ***
Comment 32 Justin Lebar (not reading bugmail) 2012-07-27 17:43:53 PDT
Try push with the resize_window test disabled:

https://tbpl.mozilla.org/?tree=Try&rev=7b93b1737715
Comment 33 Justin Lebar (not reading bugmail) 2012-07-31 20:09:46 PDT
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) 2012-07-31 20:12:31 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7c1adbb8182
Comment 35 Ed Morley [:emorley] 2012-08-01 02:51:39 PDT
https://hg.mozilla.org/mozilla-central/rev/e7c1adbb8182
Comment 36 Justin Lebar (not reading bugmail) 2012-08-01 08:45:20 PDT
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) 2012-08-01 08:50:30 PDT
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) 2012-08-01 09:10:12 PDT
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) 2012-08-01 10:39:15 PDT
Famous last words, but this might work: https://tbpl.mozilla.org/?tree=Try&rev=1ac7960758d2
Comment 40 Justin Lebar (not reading bugmail) 2012-08-02 11:17:39 PDT
Created attachment 648398 [details] [diff] [review]
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 [:bz] (still a bit busy) 2012-08-10 06:16:50 PDT
Comment on attachment 648398 [details] [diff] [review]
Part 2a: Fix aDialog parameter in nsWindowWatcher.

r=me
Comment 42 Phil Ringnalda (:philor) 2012-08-10 18:55:41 PDT
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) 2012-08-10 21:14:51 PDT
This is so nuts.
Comment 44 Justin Lebar (not reading bugmail) 2012-08-13 13:07:11 PDT
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) 2012-08-13 13:09:10 PDT
https://tbpl.mozilla.org/?tree=Try&rev=054c2a61eec8

Note You need to log in before you can comment on or make changes to this bug.