914748 - openURI should not throw exceptions in the OPEN_NEWTAB case when it fails to create a new browser

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Boris Zbarsky [:bzbarsky]]

_openURIInNewTab can return null in various cases, which gives us things like:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | uncaught exception - NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "browser is null" {file: "chrome://browser/content/browser.js" line: 13192}]' when calling method: [nsIBrowserDOMWindow::openURI] at chrome://global/content/contentAreaUtils.js:1088

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: openURI should not try to get .contentWindow on a null browser.

Comment 2

[Dão Gottwald [:dao]]

Apparently openURIInFrame has the same problem.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 802464 [details] [diff] [review]
openURI should not try to get .contentWindow on a null browser.

Ah, thank you.  Let's try the relevant reviewer!

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: openURI should not try to get .contentWindow on a null browser, and openURL should not try to focus null windows.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That last patch causes openURL to actually work, but browser_pluginnotification.js depends on it being broken: it times out with the patch.  So I guess I'll just fix it to silently not work...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: openURI should not try to get .contentWindow on a null browser, and openURL should not try to focus null windows.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 803035 [details] [diff] [review]
openURI should not try to get .contentWindow on a null browser, and openURL should not try to focus null windows.

>--- a/toolkit/content/contentAreaUtils.js
>+++ b/toolkit/content/contentAreaUtils.js
>@@ -1087,17 +1087,19 @@ function openURL(aURL)
>     protocolSvc.loadUrl(uri);
>   }
>   else {
>     var recentWindow = Services.wm.getMostRecentWindow("navigator:browser");
>     if (recentWindow) {
>       var win = recentWindow.browserDOMWindow.openURI(uri, null,
>                                                       recentWindow.browserDOMWindow.OPEN_DEFAULTWINDOW,
>                                                       recentWindow.browserDOMWindow.OPEN_NEW);
>-      win.focus();
>+      if (win) {
>+        win.focus();
>+      }
>       return;
>     }

I don't think it's ok to return here if win is null.

Also, did you see comment 2?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I don't think it's ok to return here if win is null.

What do you suggest then?  We have tests that depend on this code not pressing on past this point if window is null; see comment 5.  I tried to figure out what that test is doing, and failed.  Do you want to deal with it?

> Also, did you see comment 2?

Missed that.  I can fix that codepath, sure.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Boris Zbarsky [:bz] from comment #8)
> > I don't think it's ok to return here if win is null.
> 
> What do you suggest then?  We have tests that depend on this code not
> pressing on past this point if window is null; see comment 5.  I tried to
> figure out what that test is doing, and failed.  Do you want to deal with it?

Looks like that test causes openURL to be called from browser-plugins.js. If I replace that openURL with openUILinkIn(..., "tab"), the test passes. So I suppose the test is OK and openURL is still wonky. As far as I can tell, 'win' shouldn't even be null here, because the window this test runs in is (supposedly) a perfectly well browser window capable of opening a new tab...

Comment 10

[Dão Gottwald [:dao]]

Attachment: openURL fix

This fixes it for me. openUILinkIn never fails and also takes care of focusing the window.

I still don't what went wrong with the browserDOMWindow.openURI call, though.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Dão, thank you for picking this up!

Comment 12

[Dão Gottwald [:dao]]

Attachment: openURI and openURIInFrame fix

Comment 13

[Boris Zbarsky [:bzbarsky]]

Dão, with that first patch, I seem to get b-c test failures:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 18a, Update link should open up the plugin check page

and lots of timeouts.

Comment 14

[Dão Gottwald [:dao]]

Attachment: openURL fix v2

(In reply to Boris Zbarsky [:bz] from comment #13)
> Dão, with that first patch, I seem to get b-c test failures:
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/base/content/test/
> browser_pluginnotification.js | Test 18a, Update link should open up the
> plugin check page
> 
> and lots of timeouts.

Oops, erroneously passed 'uri' (an nsIURI) to openUILinkIn. This needs to be uri.spec.

Comment 15

[:Felipe Gomes (needinfo for replies!)]

I wasn't quite sure which patches needed review and what combination needs to land, but I looked at them both and it looks reasonable. Still strange that those calls would fail in the first place..

Comment 16

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/3ec60fd59b9c
https://hg.mozilla.org/integration/fx-team/rev/cb091c9fe504

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 804875 [details] [diff] [review]
openURL fix v2

>diff --git a/toolkit/content/contentAreaUtils.js b/toolkit/content/contentAreaUtils.js

>     if (recentWindow) {
>-      var win = recentWindow.browserDOMWindow.openURI(uri, null,
>-                                                      recentWindow.browserDOMWindow.OPEN_DEFAULTWINDOW,
>-                                                      recentWindow.browserDOMWindow.OPEN_NEW);
>-      win.focus();
>+      recentWindow.openUILinkIn(uri.spec, "tab");
>       return;
>     }

IIRC openURI was used because it didn't depend on an app-specific method like openUILinkIn. This probably broke Thunderbird (SeaMonkey seems to have it)?

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Mark, see comment 17 (and redirect as necessary)?

Comment 19

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #17)
> IIRC openURI was used because it didn't depend on an app-specific method
> like openUILinkIn. This probably broke Thunderbird (SeaMonkey seems to have
> it)?

Thunderbird doesn't have navigator:browser windows.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ec60fd59b9c
https://hg.mozilla.org/mozilla-central/rev/cb091c9fe504

Comment 21

[Mark Banner (:standard8)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #17)
> IIRC openURI was used because it didn't depend on an app-specific method
> like openUILinkIn. This probably broke Thunderbird (SeaMonkey seems to have
> it)?

(In reply to Dão Gottwald [:dao] from comment #19)
> Thunderbird doesn't have navigator:browser windows.

Yeah, that's true, I think we're fine here. Thanks for the heads-up.