1364444 - Can't receive tabs if no windows open

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Edouard Oger [:eoger]]

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

Comment 1

[Edouard Oger [:eoger]]

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

Comment 2

[Edouard Oger [:eoger]]

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.

Comment 3

[Edouard Oger [:eoger]]

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

Comment 4

[Edouard Oger [:eoger]]

Attachment: 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.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 6

[Edouard Oger [:eoger]]

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

[Edouard Oger [:eoger]]

Attachment: Bug 1364444 - Open sent tabs new windows using WindowWatcher.

Review commit: https://reviewboard.mozilla.org/r/143246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143246/

Comment 8

[Edouard Oger [:eoger]]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1688a87dda09
Open sent tabs new windows using WindowWatcher. r=eoger

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1688a87dda09

Comment 11

[Mark Hammond [:markh] [:mhammond]]

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?

Comment 12

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 13

[Edouard Oger [:eoger]]

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.