Can't receive tabs if no windows open

RESOLVED FIXED in Firefox 55

Status

()

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: eoger, Assigned: eoger)

Tracking

unspecified
Firefox 55
Unspecified
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
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

a year ago
Another funny thing: at some point a tab opened, but in a "popup" window (no toolbar, no right click).
(Assignee)

Comment 2

a year 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.
Flags: needinfo?(eoger)
(Assignee)

Comment 3

a year 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)
Assignee: nobody → eoger
Priority: -- → P1
(Assignee)

Comment 4

a year ago
Created attachment 8870938 [details] [diff] [review]
bug-1364444.patch

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 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

a year 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

a year ago
Attachment #8870938 - Attachment is obsolete: true
(Assignee)

Comment 8

a year 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+

Comment 9

a year ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1688a87dda09
Open sent tabs new windows using WindowWatcher. r=eoger
https://hg.mozilla.org/mozilla-central/rev/1688a87dda09
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 11

a year 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+
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

a year 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.