Closed
Bug 1364444
Opened 7 years ago
Closed 7 years ago
Can't receive tabs if no windows open
Categories
(Firefox :: Sync, defect, P1)
Tracking
()
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: eoger, Assigned: eoger)
Details
Attachments
(1 file, 1 obsolete file)
Found this bug on my mac this morning, try sending a tab to an open Firefox instance with no windows open (not possible on window): It seems that these lines are not doing what's expected: http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/browser/components/nsBrowserGlue.js#2144-2145
Assignee | ||
Comment 1•7 years ago
|
||
Another funny thing: at some point a tab opened, but in a "popup" window (no toolbar, no right click).
Assignee | ||
Comment 2•7 years ago
|
||
It seems like it was a transient problem, because I can't reproduce this. I'll try again on my home computer tonight, if I can't reproduce this again let's close this.
Updated•7 years ago
|
Flags: needinfo?(eoger)
Assignee | ||
Comment 3•7 years ago
|
||
I have been able to reproduce this bug.
STR:
Open Firefox, pin a tab and close the window.
> 12:37:56.856 Error displaying tab(s) received by Sync: TypeError: win is null 1 nsBrowserGlue.js:2256
Flags: needinfo?(eoger)
Updated•7 years ago
|
Assignee: nobody → eoger
Priority: -- → P1
Assignee | ||
Comment 4•7 years ago
|
||
MozReview is down, here's a patch file instead. I asked around in #fx-team and was recommended to use the WindowWatcher instead of the hidden Dom window.
Attachment #8870938 -
Flags: review?(markh)
Comment 5•7 years ago
|
||
Comment on attachment 8870938 [details] [diff] [review] bug-1364444.patch Review of attachment 8870938 [details] [diff] [review]: ----------------------------------------------------------------- Awesome, thanks! ::: browser/components/nsBrowserGlue.js @@ +2179,5 @@ > let chromeWindow = RecentWindow.getMostRecentBrowserWindow(); > chromeWindow.openPreferences(...args); > }, > > + // This method doesn't need a window object. I don't think this comment makes sense for the future (ie, it will seem like a strange somewhat random comment once this bug is fixed) @@ +2195,5 @@ > * Called as an observer when Sync's "display URIs" notification is fired. > * > * We open the received URIs in background tabs. > */ > + async _onDisplaySyncURIs(data) { you've made this async, so I think you now need a trailing .catch(Cu.reportError) at the call site.
Attachment #8870938 -
Flags: review?(markh) → review+
Assignee | ||
Comment 6•7 years ago
|
||
> you've made this async, so I think you now need a trailing .catch(Cu.reportError) at the call site.
I don't think it's needed, I tried throwing in _onDisplaySyncURIs and the error was correctly reported in the browser console.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8870938 -
Attachment is obsolete: true
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8871832 [details] Bug 1364444 - Open sent tabs new windows using WindowWatcher. https://reviewboard.mozilla.org/r/143246/#review147026 Mirroring Mark's r+ on the previous patch.
Attachment #8871832 -
Flags: review+
Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1688a87dda09 Open sent tabs new windows using WindowWatcher. r=eoger
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1688a87dda09
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8871832 [details] Bug 1364444 - Open sent tabs new windows using WindowWatcher. https://reviewboard.mozilla.org/r/143246/#review147336 ::: browser/components/nsBrowserGlue.js:2272 (Diff revision 1) > if (AppConstants.platform == "win") { > imageURL = "chrome://branding/content/icon64.png"; > } > AlertsService.showAlertNotification(imageURL, title, body, true, null, clickCallback); > } catch (ex) { > Cu.reportError("Error displaying tab(s) received by Sync: " + ex); ah, the reason you see the error reported is this catch block - however, I still think it might confuse people - calling an async function without a trailing .catch still looks like a mistake. How about removing the try/catch block and replacing it with a .catch at the call-site?
Attachment #8871832 -
Flags: review?(markh) → review+
Comment 12•7 years ago
|
||
oops - I missed the fact this landed. If there are review comments you disagree with, it's probably best to get clarification before landing. I think we should open a new bug as a followup.
Assignee | ||
Comment 13•7 years ago
|
||
No problem, I actually tested the change by removing the whole body of the function and replacing it by |throw new Error(...)| and the error was still reported in the console.
You need to log in
before you can comment on or make changes to this bug.
Description
•