1135261 - window.open() returns falsey reference from desktop app

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect, P1)

Description

[Kumar McMillan [:kumar]]

STR:
- Load this page on Firefox desktop: http://webapprt-win.paas.allizom.org/
- Click Install As App
- Open the desktop app
- Click Open A Window
- Observe the test shown

This test shows that the return value from window.open() is falsey when the example is run from a desktop app. If you run it from a desktop browser the return value from window.open() is a proper window reference, as expected.

I was testing with: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:37.0) Gecko/20100101 Firefox/37.0

Comment 1

[Kumar McMillan [:kumar]]

I just tried it on Nightly, still broken. I was on:

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:36.0) Gecko/20100101 Firefox/36.0

Comment 2

[Kumar McMillan [:kumar]]

oops, I meant this was the Nightly I tested with:

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:38.0) Gecko/20100101 Firefox/38.0

Comment 3

[Kumar McMillan [:kumar]]

Nick, do you maybe have time to help us with this? It's blocking desktop payments.

Comment 4

[(not reading bugmail) Nick Desaulniers [:\n]]

Indeed, even using remote debugging:
```
a = gAppBrowser.contentWindow.open('http://google.com')
a; // => null
```

If I instead try to run the same JS in the same build of FF nightly (it's local console, not remote debugging the running app), I get a yellow door hanger "Nightly prevented this site from opening a pop-up window." and the reference is again `null`!  I wonder if we either have door hangers explicitly disabled, or we're not showing door hangers and that is the cause?  I didn't see anything explicit in mozilla-central/webapprt/prefs.js, but who knows what some of these prefs do.

Comment 5

[(not reading bugmail) Nick Desaulniers [:\n]]

Yeah, my guess would be that we'd have to add an exception to the pop preferences, but that seems kind of brittle, since an app might load Site A, have a link to Site B which tries to pop up the payment window.  The only pref I can find that seems related is `dom.popup_allowed_events` but it's an event list.  I'm not sure if maybe we can use some kind of custom event from fxpay + pref in webapprt/prefs.js?

Comment 6

[(not reading bugmail) Nick Desaulniers [:\n]]

MattN pointed me at this code [0] for how FF handles pop up windows.  We'd probably have to add XUL to webapp/content/webapp.xul to reimplement the yellow door hanger, or equivalent.  The one implication being, how does one then prevent windows from being opened?  In general, I feel like we should instead be intercepting the blocking and allowing it for a whitelist of urls that are accessed by fxpay.  Bill brings up that we did open windows from a runtime before, but the issue being that we don't have valid references to them.  I might be on the wrong track though with the pop up blocker, since a window does open, just the call to window.open returns null.

[0] https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#459

Comment 7

[Kumar McMillan [:kumar]]

Wait, I'm confused. When you run the test code I linked to, the popup *does* open. It's just that there's no reference to it in the code. Why do we need a door hanger?

Comment 8

[(not reading bugmail) Nick Desaulniers [:\n]]

Actually, I probably want to step through:
mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#7636
mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#7664
mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#11729

Comment 9

[Kumar McMillan [:kumar]]

This may not be the same thing happening on Android. It would need some investigation. On Android the window doesn't open at all which makes me suspect that it's a synthetic event issue.

Comment 10

[(not reading bugmail) Nick Desaulniers [:\n]]

Oh man, I spent all day debugging this with GDB and staring at C++ code.  It took me a while to realize the difference between desktop Firefox and WebRT was that WebRT is creating a dialog box, and when I dumped the JS stack trace, I saw that indeed we're intercepting calls to window.open and replacing them with calls to window.openDialog.  I'm still not sure yet if this is the error, or why we're failing to return a reference, but doing this is the divergent code path from desktop FF and I think it's related.

http://mxr.mozilla.org/mozilla-central/source/webapprt/content/webapp.js#121
http://mxr.mozilla.org/mozilla-central/source/webapprt/content/webapp.js#74
http://mxr.mozilla.org/mozilla-central/source/webapprt/content/webapp.js#86

Comment 11

[Bill Walker [:bwalker] [@wfwalker]]

(In reply to Nick Desaulniers [:\n] from comment #10)
> Oh man, I spent all day debugging this with GDB and staring at C++ code.  It
> took me a while to realize the difference between desktop Firefox and WebRT
> was that WebRT is creating a dialog box, and when I dumped the JS stack
> trace, I saw that indeed we're intercepting calls to window.open and
> replacing them with calls to window.openDialog.  I'm still not sure yet if
> this is the error, or why we're failing to return a reference, but doing
> this is the divergent code path from desktop FF and I think it's related.
> 
> http://mxr.mozilla.org/mozilla-central/source/webapprt/content/webapp.js#121
> http://mxr.mozilla.org/mozilla-central/source/webapprt/content/webapp.js#74
> http://mxr.mozilla.org/mozilla-central/source/webapprt/content/webapp.js#86

Nick, it looks to me like you need to talk to Marco about his fix for bug 847518

Comment 12

[Marco Castelluccio [:marco]]

This comment explains why I used window.openDialog instead of window.open: https://bugzilla.mozilla.org/show_bug.cgi?id=847518#c28

I think the problem is actually unrelated to window.openDialog vs window.open: https://bugzilla.mozilla.org/show_bug.cgi?id=847518#c4

Comment 13

[(not reading bugmail) Nick Desaulniers [:\n]]

Marco, what bug did you file from this comment? https://bugzilla.mozilla.org/show_bug.cgi?id=847518#c32

In regards to your statement "I haven't dug deep, but looks like both window.open and the WW service use nsIWindowProvider while window.openDialog doesn't, so probably there's a bug there." in comment 28.

Comment 14

[Marco Castelluccio [:marco]]

> Marco, what bug did you file from this comment? https://bugzilla.mozilla.org/show_bug.cgi?id=847518#c32

I don't recall if I filed a bug for it, I probably didn't.

Seems like to fix this bug we should either find a workaround for https://bugzilla.mozilla.org/show_bug.cgi?id=847518#c4 or stop using mozbrowser altogether.

Comment 15

[(not reading bugmail) Nick Desaulniers [:\n]]

we're definitely failing an assertion here [0]:

  NS_ENSURE_TRUE(domReturn, NS_OK);

which is preventing us from calling the very necessary:

  domReturn.swap(*aReturn);

What's tricky is that we intercept opening a window and open a window, so if we set a break point, we hit this twice, the first is when we call window.openDialog and the second is the client side JS' call to window.open (which is the one we're interested in).  Using this gdb macro is really helpful, see How do I display PRUnichar's? [1].

I need to figure out why domReturn (which is a nsCOMPtr<nsIDOMWindow>) .mRawPtr is null after a call to OpenWindow2. [2]

[0] mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#11882
[1] https://developer.mozilla.org/en-US/docs/Debugging_Mozilla_with_gdb
[2] mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#11849

Comment 16

[(not reading bugmail) Nick Desaulniers [:\n]]

  *_retval = 0;

This should definitely be:

  *_retval = nullptr;

but is not the source of our bug.

[0] http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/nsWindowWatcher.cpp#462

Comment 17

[(not reading bugmail) Nick Desaulniers [:\n]]

We're taking this branch [0], returning NS_OK though *_retval is still nullptr.  I need to understand this cryptic comment, 

// NS_ERROR_ABORT means the window provider has flat-out rejected
// the open-window call and we should bail.  Don't return an error
// here, because our caller may propagate that error, which might
// cause e.g. window.open to throw!  Just return null for our out
// param.

from this commit [1] and this bug [2] "Handle window.open in <iframe mozbrowser>".  Seems relevant. ;)

[0] http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/nsWindowWatcher.cpp#645
[1] http://hg.mozilla.org/mozilla-central/diff/65a425a98e86/embedding/components/windowwatcher/src/nsWindowWatcher.cpp
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=742944

Comment 18

[(not reading bugmail) Nick Desaulniers [:\n]]

Spoke to khuey on Tuesday, he sugguested I speak to Olli.  @Olli, any idea what's up with the window provider, or what the best way to approach fixing this might be?

Comment 19

[Andy McKay]

Any progress on this?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Nick, is the relevant code entering 
http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsContentTreeOwner.cpp#822
Where do we get the NS_ERROR_ABORT?
We shouldn't enter http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsContentTreeOwner.cpp#854

Comment 21

[(not reading bugmail) Nick Desaulniers [:\n]]

We do enter that conditional (line 854) so all of those conditions are true.

We're returning NS_ERROR_ABORT here [0], and interestingly:

(gdb) p *aWindowIsNew
$4 = false
(gdb) p opened
$5 = mozilla::BrowserElementParent::OPEN_WINDOW_CANCELLED

So maybe a bug in BrowserElementParent::OpenWindowInProcess, but the comments in nsContentTreeOwner::ProvideWindow seem highly relevant.

In BrowserElementParent::OpenWindowInProcess, we fail this check [1] because:
(gdb) p opened
$7 = mozilla::BrowserElementParent::OPEN_WINDOW_IGNORED

In BrowserElementParent::DispatchOpenWindowEvent, we fail at [2]:
(gdb) p aPopupFrameElement->IsInDoc()
$12 = false
(gdb) p status
$13 = nsEventStatus_eConsumeNoDefault

I really don't understand the implications of all of these conditionals.  Is there something obviously wrong here, Olli?

[0] http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsContentTreeOwner.cpp#868
[1] http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.cpp#287
[2] http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.cpp#192

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Well, why do we enter 854? docshell->GetIsInBrowserOrApp() returning true outside b2g feels rather odd.
Why are we using BrowserElementParent anywhere here?
Hmm, apparently I know nothing about the setup of webapprt. Very surprising to see mozbrowser element usage there.
If mozbrowser can open windows properly in b2g, follow the same code paths in webapprt?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(I thought webapprt would be more like a top level xul document having one <xul:browser type="content"> in it)

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Isn't the issue that 
http://mxr.mozilla.org/mozilla-central/source/webapprt/content/webapp.js#74 never adds
event.detail.frameElement to any document, so http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.cpp#190 is false.

In case of Gaia, the iframe element is appended to a container somewhere in http://mxr.mozilla.org/gaia/source/apps/system/js/app_window.js, if I read that code correctly.

Comment 25

[(not reading bugmail) Nick Desaulniers [:\n]]

Yes, tracing the gaia code:

* ChildWindowFactory listens for "mozbrowseropenwindow": http://mxr.mozilla.org/gaia/source/apps/system/js/child_window_factory.js#36
* handleEvent calls to createPopupWindow: http://mxr.mozilla.org/gaia/source/apps/system/js/child_window_factory.js#109
* A PopupWindow is constructed: http://mxr.mozilla.org/gaia/source/apps/system/js/child_window_factory.js#132
* PopupWindow inherits from AppWindow: http://mxr.mozilla.org/gaia/source/apps/system/js/popup_window.js#23
* iframe is stored in this.browser.element: http://mxr.mozilla.org/gaia/source/apps/system/js/app_window.js#632
* this.browser.element appended to DOM: http://mxr.mozilla.org/gaia/source/apps/system/js/app_window.js#665

So maybe here: http://mxr.mozilla.org/mozilla-central/source/webapprt/content/webapp.js#74, we need to append event.detail.frameElement to document.

The differences from HTML and XUL are preventing me from implementing something that should be so basic.  Something like document.body.appendChild(event.detail.frameElement); doesn't work. :(

Comment 26

[(not reading bugmail) Nick Desaulniers [:\n]]

(In reply to Nick Desaulniers [:\n] from comment #25)
> The differences from HTML and XUL are preventing me from implementing
> something that should be so basic.  Something like
> document.body.appendChild(event.detail.frameElement); doesn't work. :(

Nvm, I've done this before, in another bug.

Comment 27

[(not reading bugmail) Nick Desaulniers [:\n]]

And that works!  We just need to hide the iframe that's appended to webapps.xul.  Kumar, can you add a button to your test app that opens a window with some content, in addition to the existing button?  I need to check that if I hide the iframe appended to the XUL document, that it doesn't mess up the content in the external window.  Also, if the window is closed, maybe we need to remove that element?

Comment 28

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: Bug 1135261 - window.open() returns falsey reference from desktop app

Comment 29

[(not reading bugmail) Nick Desaulniers [:\n]]

Actually, looks like we don't want to remove the element?

JavaScript error: chrome://webapprt/content/webapp.js, line 96: NotFoundError: Node was not found

Comment 30

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: Bug 1135261 - window.open() returns falsey reference from desktop app

Comment 31

[(not reading bugmail) Nick Desaulniers [:\n]]

Indeed, this is not quite right.  For instance, Kumar just updated his app to open a window to a site that auto plays music (http://nyan.cat).  When the music is loaded, you hear it playing twice, once in the new window, and once in the hidden iframe in the XUL window.  After you close the newly opened window, the music keeps playing in the hidden iframe (so it's not auto-magically removed) though just once, not twice.

Comment 32

[(not reading bugmail) Nick Desaulniers [:\n]]

Harald also mentions the case for authentication, where we have one time tokens.  If the client side JS (in Kumar's app, for instance) gets a window reference, is it a reference to the iframe hidden within the XUL window, or the new window that opened?

Comment 33

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8589765 [details] [diff] [review]
Bug 1135261 - window.open() returns falsey reference from desktop app

Per comment 24 and bug 847518, comment 4, I don't think appending the frame element to the DOM of the opener window will work.  It does make the window.open call return a window object, but that object is the one associated with that frame element, not the one in the dialog that onOpenWindow opens.

And attaching the frame element to the DOM in addition to opening a new window and loading the URL in it results in loading the URL twice, as you discovered.

Presumably the mozbrowseropenwindow event is intended to be used by web apps, not XUL apps, and thus was designed for apps to be able to "open" windows by adding their frames to their existing DOM rather than by calling a chrome API like window.openDialog to open a new native window.

Note also that onOpenWindow responds to the mozbrowseropenwindow event by opening a new native window, loading webapp.xul in it, and then telling webapp.xul to load the specified URL.  And webapp.xul loads asynchronously.  So onOpenWindow has already returned before the onLoad callback inside onOpenWindow ever gets a chance to load the URL, by which time presumably the mozbrowseropenwindow event dispatch has already completed, and the window.open call that triggered it has already returned.

I think we need to figure out how Firefox handles window.open calls, and do the same thing it does, as mozbrowseropenwindow was designed for web apps, and the runtime is a XUL app, like Firefox.  I can imagine a hack or two that might work around the problem with mozbrowseropenwindow, but then we're just layering hack upon hack.  It's time to figure the correct way to open windows in the runtime.

Comment 34

[Kohei Yoshino [:kohei]]

(In reply to Kumar McMillan [:kumar] (needinfo all the things) from comment #9)
> This may not be the same thing happening on Android. It would need some
> investigation. On Android the window doesn't open at all which makes me
> suspect that it's a synthetic event issue.

I have encountered this issue on my BzDeck app. window.open() doesn't work on the Android WebAppRT.

https://github.com/bzdeck/bzdeck/issues/293

Comment 35

[Michael Ellis]

@ Bill - Would you mind taking a look at this issue and see if we need to address it in the WRT?

Comment 36

[Kohei Yoshino [:kohei]]

Filed Bug 1183897 for the Android issue.

Comment 37

[Myk Melez [:myk] [@mykmelez]]

Attachment: fix mozbrowseropenwindow default handling

(In reply to Olli Pettay [:smaug] from comment #22)
> Well, why do we enter 854? docshell->GetIsInBrowserOrApp() returning true
> outside b2g feels rather odd.
> Why are we using BrowserElementParent anywhere here?
…
(In reply to Olli Pettay [:smaug] from comment #23)
> (I thought webapprt would be more like a top level xul document having one
> <xul:browser type="content"> in it)

The runtime doesn't actually use an <iframe mozbrowser>, it uses a XUL <browser type="content-primary">.  But it sets an app ID on the docShell, since it loads an app into that frame, and that causes docshell->GetIsInBrowserOrApp() to return true, which makes nsContentTreeOwner::ProvideWindow call BrowserElementParent::OpenWindowInProcess to dispatch a mozbrowseropenwindow event.

That shouldn't be a problem, since we should be able to simply ignore the event to make nsContentTreeOwner::ProvideWindow create a new window and return it to the window.open caller.  But BrowserElementParent::DispatchOpenWindowEvent calls DispatchCustomDOMEvent to dispatch the mozbrowseropenwindow event.  And DispatchCustomDOMEvent initializes *status* to nsEventStatus_eConsumeNoDefault.  So this branch of BrowserElementParent::DispatchOpenWindowEvent is always reached, regardless of whether or not preventDefault was called:

    } else if (status == nsEventStatus_eConsumeNoDefault) {
      // If the frame was not added to a document, report to callers whether
      // preventDefault was called on or not
      return BrowserElementParent::OPEN_WINDOW_CANCELLED;

And thus nsContentTreeOwner::ProvideWindow never tries to create a native window.

(The only other caller of DispatchCustomDOMEvent, DispatchAsyncScrollEventRunnable::Run, has the same problem.  It explicitly initializes *status* to nsEventStatus_eIgnore, but DispatchCustomDOMEvent overrides that.)


The fix is to initialize *status* to nsEventStatus_eIgnore, so it only becomes nsEventStatus_eConsumeNoDefault if preventDefault is actually called.  Then the runtime's mozbrowseropenwindow handler can simply do nothing to make nsContentTreeOwner::ProvideWindow open a new window, load the URL into it, and return it to the window.open caller.

Here's a patch that fixes the bug.  Requesting review from :smaug for the BrowserElementParent changes and :marco for the runtime changes.

Marco: now that onOpenWindow no longer creates the window itself, it can't hide the menubar nor set the app ID, so I had to move that code to other places.  I think the app ID change is ok (it now gets set in a DOMContentLoaded handler for the new XUL window), but I'm less sure about hiding the menubar on the location change.  That deserves some further thought.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8635643 [details] [diff] [review]
fix mozbrowseropenwindow default handling

Hmm, https://bugzilla.mozilla.org/show_bug.cgi?id=826325#c42 asked to do what you do here, but something else was then pushed to m-i?

The changes to BrowserElementParent look ok to me.

Comment 39

[Marco Castelluccio [:marco]]

Comment on attachment 8635643 [details] [diff] [review]
fix mozbrowseropenwindow default handling

Review of attachment 8635643 [details] [diff] [review]:
-----------------------------------------------------------------

Have you run the webapprt-chrome tests on all platforms?

::: webapprt/content/webapp.js
@@ +102,4 @@
>  
> +function onDOMContentLoaded() {
> +  window.removeEventListener("DOMContentLoaded", onDOMContentLoaded, false);
> +    // The initial window's app ID is set by Startup.jsm before the app

I agree with you this should be OK, unless the platform code is doing something with the app ID before this event is triggered.

@@ +105,5 @@
> +    // The initial window's app ID is set by Startup.jsm before the app
> +    // is loaded, so this code only handles subsequent windows that are opened
> +    // by the app via window.open calls.  We do this on DOMContentLoaded
> +    // in order to ensure it gets set before the window's content is loaded.
> +    if (gAppBrowser.docShell.appId === Ci.nsIScriptSecurityManager.NO_APP_ID) {

Nit: the indentation here is off

Comment 40

[Myk Melez [:myk] [@mykmelez]]

(In reply to Marco Castelluccio [:marco] from comment #39)

> Have you run the webapprt-chrome tests on all platforms?

I've run them locally on Windows, Mac, and Linux x64 (Ubuntu), and they all pass there.  But I've pushed this to tryserver to ensure the browser-element tests pass on all the other platforms too:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37636e33b41b


> ::: webapprt/content/webapp.js

> @@ +105,5 @@
> > +    // The initial window's app ID is set by Startup.jsm before the app
> > +    // is loaded, so this code only handles subsequent windows that are opened
> > +    // by the app via window.open calls.  We do this on DOMContentLoaded
> > +    // in order to ensure it gets set before the window's content is loaded.
> > +    if (gAppBrowser.docShell.appId === Ci.nsIScriptSecurityManager.NO_APP_ID) {
> 
> Nit: the indentation here is off

Got it, I'll fix that before pushing.

Comment 41

[Myk Melez [:myk] [@mykmelez]]

Attachment: close new window in widget tests

The browser-element tests passed in the tryserver run, but dom/apps/tests/test_widget_browser.html failed with errors like:

INFO -  282 INFO TEST-UNEXPECTED-FAIL | dom/apps/tests/test_widget_browser.html | Error callback invoked - expected PASS

- http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-37636e33b41b/try-linux/try_ubuntu32_vm_test-mochitest-1-bm06-tests1-linux32-build1467.txt.gz

That happens because dom/apps/tests/file_test_widget.js calls window.open(), which now opens a new window instead of doing nothing.  And the new window (a tab on desktop) triggers a mochitest "unable to restore focus" failure.

I don't think the test is actually testing that window.open() does nothing.  It looks like it simply relied on that behavior, which happened because mozbrowseropenwindow event would always appear to have had preventDefault() called on it.  Now that Gecko handles an unhandled window.open() call by opening a new window, the fix for the test failure is for the test to close the new window after opening it.

But it'd be good to get confirmation from someone familiar with that test, so requesting review on the dom/apps/tests/file_test_widget.js change from :junior, who initially implemented it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2528ffd45183

Comment 42

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8636238 [details] [diff] [review]
close new window in widget tests

Erm, but I didn't mean to cancel the wesj review request.  Reinstating it.

Comment 43

[Junior [inactive]]

Comment on attachment 8636238 [details] [diff] [review]
close new window in widget tests

Review of attachment 8636238 [details] [diff] [review]:
-----------------------------------------------------------------

r=me only for |file_test_widget.js|
Looks good to me.
FWIW, the reason to trigger |window.open| is to check not leaking |mozbrowseropenwindow| event to |mozWidget| embedder.

Comment 44

[Myk Melez [:myk] [@mykmelez]]

Here's the most recent try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=145a59736497

Comment 45

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35c6e7e9c205

Comment 46

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11980466&repo=mozilla-inbound

Comment 47

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98c2f8a27530

Comment 48

[Myk Melez [:myk] [@mykmelez]]

I haven't had any luck reproducing test failures specific to this branch so far.  If I run the specific test that failed in the build referenced by comment 46, I don't get a failure.  If I run all tests for "verticalhome," on the other hand, I do see two failures due to "Crash detected but error running stackwalk".  But I get the same failures when I test without my changes.  So I'm not sure how to isolate failures that are specific to my changes.  But I'll keep working on it.

Comment 49

[Myk Melez [:myk] [@mykmelez]]

I still can't reproduce the test failures, neither on Linux x64 nor on Mac.  So I've merged the latest changes from Central and re-pushed to try to see if it's been addressed by an upstream change:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=137f1d4f4114

Comment 50

[Myk Melez [:myk] [@mykmelez]]

It hasn't been addressed by an upstream change, as the same test failures reoccurred on that try run, and they all pass on this try run against an unmodified Central:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54889b8d546b

Comment 51

[Myk Melez [:myk] [@mykmelez]]

I still can't reproduce locally, but I realized that the test failure could be caused by mozbrowseropenwindow handlers in Gaia relying on the buggy behavior and not calling preventDefault().  So I fixed all such handlers in bug 1192489, and this try run with those fixes confirms that they resolve the test failures:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=005dbab8c4b2

Comment 52

[Myk Melez [:myk] [@mykmelez]]

Attachment: update to tip of Central branch

Here's an updated patch that applies to the tip of Central (resolving trivial conflicts), and I've kicked off one more tryserver run, now that bug 1192489 has landed, to ensure that tests now pass as expected:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ce6f47a89ff

Comment 53

[Myk Melez [:myk] [@mykmelez]]

Linux opt M-e10s(bc2) kept failing in the tryserver run, so I did this run of the Central branch without these changes (at the Central commit on which I applied these changes):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a2fc315545d

It shows the same persistent failure in that suite.  And all other suites passed (or failed due to known intermittent failures and then passed upon a rerun).  So this is ready to reland.

Comment 54

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e38c128b59a

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4e38c128b59a