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

RESOLVED FIXED in Firefox 26

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bz, Assigned: dao)

Tracking

({regression})

unspecified
Firefox 26
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

_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
Created attachment 802464 [details] [diff] [review]
openURI should not try to get .contentWindow on a null browser.
Attachment #802464 - Flags: review?(dolske)
Whiteboard: [need review]
(Assignee)

Updated

5 years ago
Blocks: 902695
Keywords: regression
(Assignee)

Comment 2

5 years ago
Apparently openURIInFrame has the same problem.
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!
Attachment #802464 - Flags: review?(dolske) → review?(felipc)
Created attachment 802799 [details] [diff] [review]
openURI should not try to get .contentWindow on a null browser, and openURL should not try to focus null windows.
Attachment #802799 - Flags: review?(felipc)
Attachment #802464 - Attachment is obsolete: true
Attachment #802464 - Flags: review?(felipc)
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...
Created 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.
Attachment #803035 - Flags: review?(felipc)
Attachment #802799 - Attachment is obsolete: true
Attachment #802799 - Flags: review?(felipc)
Attachment #803035 - Flags: review?(felipc) → review+
(Assignee)

Comment 7

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

Comment 9

5 years ago
(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...
Flags: needinfo?(dao)
(Assignee)

Updated

5 years ago
Blocks: 728749
(Assignee)

Comment 10

5 years ago
Created attachment 803746 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
Attachment #803746 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #803746 - Flags: review+ → review?(felipc)
Assignee: bzbarsky → dao
Dão, thank you for picking this up!
Attachment #803746 - Flags: review?(felipc) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 804323 [details] [diff] [review]
openURI and openURIInFrame fix
Attachment #803035 - Attachment is obsolete: true
Attachment #804323 - Flags: review?(felipc)
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.
(Assignee)

Comment 14

5 years ago
Created attachment 804875 [details] [diff] [review]
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.
Attachment #803746 - Attachment is obsolete: true
Attachment #804323 - Flags: review?(felipc) → review+
Attachment #804875 - Flags: review+
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 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)?
Mark, see comment 17 (and redirect as necessary)?
Flags: needinfo?(mbanner)
(Assignee)

Comment 19

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
https://hg.mozilla.org/mozilla-central/rev/3ec60fd59b9c
https://hg.mozilla.org/mozilla-central/rev/cb091c9fe504
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Flags: needinfo?(mbanner)
You need to log in before you can comment on or make changes to this bug.