Closed
Bug 914748
Opened 11 years ago
Closed 11 years ago
openURI should not throw exceptions in the OPEN_NEWTAB case when it fails to create a new browser
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 26
People
(Reporter: bzbarsky, Assigned: dao)
References
Details
(Keywords: regression)
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
Felipe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Felipe
:
review+
|
Details | Diff | Splinter Review |
_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
Reporter | ||
Comment 1•11 years ago
|
||
Attachment #802464 -
Flags: review?(dolske)
Reporter | ||
Updated•11 years ago
|
Whiteboard: [need review]
Assignee | ||
Updated•11 years ago
|
Blocks: 902695
Keywords: regression
Assignee | ||
Comment 2•11 years ago
|
||
Apparently openURIInFrame has the same problem.
Reporter | ||
Comment 3•11 years ago
|
||
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)
Reporter | ||
Comment 4•11 years ago
|
||
Attachment #802799 -
Flags: review?(felipc)
Reporter | ||
Updated•11 years ago
|
Attachment #802464 -
Attachment is obsolete: true
Attachment #802464 -
Flags: review?(felipc)
Reporter | ||
Comment 5•11 years ago
|
||
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...
Reporter | ||
Comment 6•11 years ago
|
||
Attachment #803035 -
Flags: review?(felipc)
Reporter | ||
Updated•11 years ago
|
Attachment #802799 -
Attachment is obsolete: true
Attachment #802799 -
Flags: review?(felipc)
Updated•11 years ago
|
Attachment #803035 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 7•11 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-
Reporter | ||
Comment 8•11 years ago
|
||
> 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•11 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 | ||
Comment 10•11 years ago
|
||
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•11 years ago
|
Attachment #803746 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #803746 -
Flags: review+ → review?(felipc)
Reporter | ||
Updated•11 years ago
|
Assignee: bzbarsky → dao
Reporter | ||
Comment 11•11 years ago
|
||
Dão, thank you for picking this up!
Updated•11 years ago
|
Attachment #803746 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #803035 -
Attachment is obsolete: true
Attachment #804323 -
Flags: review?(felipc)
Reporter | ||
Comment 13•11 years ago
|
||
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•11 years ago
|
||
(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
Updated•11 years ago
|
Attachment #804323 -
Flags: review?(felipc) → review+
Updated•11 years ago
|
Attachment #804875 -
Flags: review+
Comment 15•11 years ago
|
||
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..
Assignee | ||
Comment 16•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/3ec60fd59b9c https://hg.mozilla.org/integration/fx-team/rev/cb091c9fe504
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [need review]
Comment 17•11 years ago
|
||
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•11 years ago
|
||
Mark, see comment 17 (and redirect as necessary)?
Flags: needinfo?(mbanner)
Assignee | ||
Comment 19•11 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.
Comment 20•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3ec60fd59b9c https://hg.mozilla.org/mozilla-central/rev/cb091c9fe504
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Comment 21•11 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)? (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.
Description
•