Closed Bug 769254 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla17
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [dev-doc-needed see comment 12])

Attachments

(6 files, 1 obsolete file)

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!
Dietrich, Chris - Looks like a basecamp blocker, right?
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
OS: Mac OS X → Gonk
Hardware: x86 → ARM
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.
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 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.
Attachment #638343 - Flags: review?(mounir) → review?(bzbarsky)
Attachment #638344 - Flags: review?(mounir) → review?(bzbarsky)
Attachment #638345 - Flags: review?(mounir) → review?(bzbarsky)
Attachment #638346 - Flags: review?(mounir) → review?(bzbarsky)
Summary: Clicking a target=_blank link inside <iframe mozbrowser> crashes Gecko → Clicking a target=_blank link inside <iframe mozbrowser> crashes Gecko, should pass opened URL to mozbrowseropenwindow event
Duplicate of this bug: 764707
blocking-basecamp: ? → +
blocking-kilimanjaro: ? → +
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?
Attachment #638343 - Flags: review?(bzbarsky) → review+
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.
Attachment #638344 - Flags: review?(bzbarsky) → review+
Comment on attachment 638345 [details] [diff] [review]
Part 3: Rename nsWindowWatcher::OpenWindowJSInternal --> OpenWindowInternal.

r=me
Attachment #638345 - Flags: review?(bzbarsky) → review+
Comment on attachment 638346 [details] [diff] [review]
Part 4: Tests for clicking _blank link from <iframe mozbrowser>.

r=me
Attachment #638346 - Flags: review?(bzbarsky) → review+
> 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 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.
Attachment #638344 - Flags: review+ → review-
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
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. ....)
Attached patch Part 2, v2Splinter Review
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.
Attachment #639922 - Flags: review?(bzbarsky)
Attachment #638344 - Attachment is obsolete: true
Assignee: nobody → justin.lebar+bug
Status: NEW → ASSIGNED
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!  :)
Depends on: 772076
Comment on attachment 639922 [details] [diff] [review]
Part 2, v2

r=me.  Thanks for the interdiff!  That made life much simpler.
Attachment #639922 - Flags: review?(bzbarsky) → review+
Keywords: dev-doc-needed
Whiteboard: [dev-doc-needed see comment 12]
This is going to be fun to debug without working tryserver.  :(
> 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.
Maybe I messed up the args code?  That's my best guess at this point.

https://tbpl.mozilla.org/?tree=Try&rev=baff55f8d945
Depends on: 775676
Depends on: 677841
Depends on: 776801
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
(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.
Duplicate of this bug: 777164
Try push with the resize_window test disabled:

https://tbpl.mozilla.org/?tree=Try&rev=7b93b1737715
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7c1adbb8182
Whiteboard: [dev-doc-needed see comment 12] → [dev-doc-needed see comment 12][leave open]
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.
FWIW git blame shows that the args were set this way when ShowModalDialog was originally written, back in bug 194404.  :)
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).
Famous last words, but this might work: https://tbpl.mozilla.org/?tree=Try&rev=1ac7960758d2
Whiteboard: [dev-doc-needed see comment 12][leave open] → [dev-doc-needed see comment 12]
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
Attachment #648398 - Flags: review?(bzbarsky)
Attachment #638343 - Flags: checkin+
Comment on attachment 648398 [details] [diff] [review]
Part 2a: Fix aDialog parameter in nsWindowWatcher.

r=me
Attachment #648398 - Flags: review?(bzbarsky) → review+
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.
This is so nuts.
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...
https://hg.mozilla.org/mozilla-central/rev/b9d4cc7af705
https://hg.mozilla.org/mozilla-central/rev/5bda15ef0b4c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Keywords: verifyme
QA Contact: jsmith
Status: RESOLVED → VERIFIED
Keywords: verifyme
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.