The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla17

Status

()

Core
DOM
VERIFIED FIXED
5 years ago
4 years ago

People

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

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla17
ARM
Gonk (Firefox OS)
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

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

Attachments

(6 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
See https://github.com/mozilla-b2g/gaia/issues/1976.  Ouch.
(Assignee)

Comment 1

5 years ago
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: --- → ?

Updated

5 years ago
OS: Mac OS X → Gonk
Hardware: x86 → ARM
(Assignee)

Comment 3

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

Comment 4

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

Comment 5

5 years ago
Created attachment 638343 [details] [diff] [review]
Part 1, v1: Fix crashes in BrowserElementParent.cpp, TabChild.cpp on null URI to window-opening code.
Attachment #638343 - Flags: review?(mounir)
(Assignee)

Comment 6

5 years ago
Created attachment 638344 [details] [diff] [review]
Part 2: Pass in the URL for target=_blank links.
Attachment #638344 - Flags: review?(mounir)
(Assignee)

Comment 7

5 years ago
Created attachment 638345 [details] [diff] [review]
Part 3: Rename nsWindowWatcher::OpenWindowJSInternal --> OpenWindowInternal.
Attachment #638345 - Flags: review?(mounir)
(Assignee)

Comment 8

5 years ago
Created attachment 638346 [details] [diff] [review]
Part 4: Tests for clicking _blank link from <iframe mozbrowser>.
Attachment #638346 - Flags: review?(mounir)
(Assignee)

Comment 9

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

Updated

5 years ago
Attachment #638344 - Flags: review?(mounir) → review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #638345 - Flags: review?(mounir) → review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #638346 - Flags: review?(mounir) → review?(bzbarsky)
(Assignee)

Updated

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

Updated

5 years ago
Duplicate of this bug: 764707

Updated

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

Comment 15

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

Comment 16

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

Comment 17

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

Comment 18

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

Comment 20

5 years ago
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.
Attachment #639922 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #638344 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
Created attachment 639923 [details] [diff] [review]
Interdiff part 2, v1 -> v2
Assignee: nobody → justin.lebar+bug
Status: NEW → ASSIGNED
(Assignee)

Comment 22

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

Updated

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

Updated

5 years ago
Keywords: dev-doc-needed
Whiteboard: [dev-doc-needed see comment 12]
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
(Assignee)

Comment 25

5 years ago
This is going to be fun to debug without working tryserver.  :(
(Assignee)

Comment 26

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

Comment 27

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

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

Updated

5 years ago
Depends on: 775676
(Assignee)

Comment 28

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

Updated

5 years ago
Depends on: 677841
(Assignee)

Updated

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

Updated

5 years ago
Duplicate of this bug: 777164
(Assignee)

Comment 32

5 years ago
Try push with the resize_window test disabled:

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

Comment 33

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

Comment 34

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7c1adbb8182
Whiteboard: [dev-doc-needed see comment 12] → [dev-doc-needed see comment 12][leave open]
https://hg.mozilla.org/mozilla-central/rev/e7c1adbb8182
(Assignee)

Comment 36

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

Comment 37

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

Comment 38

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

Comment 39

5 years ago
Famous last words, but this might work: https://tbpl.mozilla.org/?tree=Try&rev=1ac7960758d2
(Assignee)

Updated

5 years ago
Whiteboard: [dev-doc-needed see comment 12][leave open] → [dev-doc-needed see comment 12]
(Assignee)

Comment 40

5 years ago
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
Attachment #648398 - Flags: review?(bzbarsky)
(Assignee)

Updated

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

Comment 43

5 years ago
This is so nuts.
(Assignee)

Comment 44

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

Comment 45

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=054c2a61eec8
https://hg.mozilla.org/mozilla-central/rev/b9d4cc7af705
https://hg.mozilla.org/mozilla-central/rev/5bda15ef0b4c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
Keywords: verifyme
QA Contact: jsmith

Updated

5 years ago
Status: RESOLVED → VERIFIED
Keywords: verifyme
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.