1365780 - After restore, most window titles say "Nightly" instead of tab title

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

After a recent Nightly update, most of my browser windows stopped displaying the title of the current tab.

In the Window menu, most windows just say "Nightly" instead of the tab title.  If I go to such a browser window, the window's title is also "Nightly".

If I change to some other tab in such a window, the window's title becomes correct for the new tab in both the window and window menu.

I am on macOS 10.12.5 with Nightly 2017-05-17.

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]:

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5872d9137b5189b020a736dd5c8fb6c9aaec2d66&tochange=9f3f63f8acb7f71ecda9d6d44cdad3af54fb9944

Regressed by:9f3f63f8acb7	Dão Gottwald — Bug 1364127 - Set the initial tab label to the URL for new tabs and to the saved title for restored tabs, and make sure that label doesn't subsequently get clobbered with a placeholder r=past



Lack of Automate test :(

Comment 2

[Mike de Boer [:mikedeboer]]

(In reply to Alice0775 White from comment #1)
> Lack of Automate test :(

True, it's very hard to catch these kind of things when reviewing and/ or when not working on Windows by default.

Comment 3

[Dão Gottwald [:dao]]

Attachment: Bug 1365780 - Consistently dispatch the TabAttrModified event and update the title bar when setting tab labels.

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

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8868961 [details]
Bug 1365780 - Consistently dispatch the TabAttrModified event and update the title bar when setting tab labels.

https://reviewboard.mozilla.org/r/140630/#review144504

Thanks for the super quick turn-around, Dão! r=me with nits fixed and a green try run, of course ;)

I added a mini-rant below, please take it as you like.

::: browser/base/content/tabbrowser.xml:1492
(Diff revision 1)
> +            if (!aLabel || aTab.getAttribute("label") == aLabel) {
>                return false;
> +            }
> +
> +            aTab.setAttribute("label", aLabel);
> +            aTab._labelIsContentTitle = aOptions && aOptions.isContentTitle;

I'm kind of starting to dislike the mixed bag and sheer amount of properties, attributes and other state-tracking-variables we mix throughout the binding and at this rate it's only going to be more.
I think the only way out of this is to start moving all tabbrowser JS to a JSM and refactor all state atoms into a single cache/ map for tabs and linked browsers.

(Aside: this would also allow us to ESLint the whole thing to enforce coding style, like the bracing changes you made. Into dedicated commits as well.)

Because, tbh here, when I read through this patch my feeling was more like 'yuck' and then like 'le sigh, there's no way around this, I guess...'

::: browser/components/sessionstore/test/browser_tab_label_during_restore.js:65
(Diff revision 1)
>    browserLoadedPromise = BrowserTestUtils.browserLoaded(secondTab.linkedBrowser, false, TEST_URL);
>    gBrowser.selectedTab = secondTab;
>    await browserLoadedPromise;
>    ok(!secondTab.hasAttribute("pending"), "second tab isn't pending anymore");
> -  is(gBrowser.selectedTab.label, CONTENT_TITLE, "second tab displays content title");
> +  is(secondTab.label, CONTENT_TITLE, "second tab displays content title");
> +  is(document.title.indexOf(CONTENT_TITLE), 0, "title bar displays content title");

nit: `ok("".startsWith(CONTENT_TITLE), "");`

::: browser/components/sessionstore/test/browser_tab_label_during_restore.js:73
(Diff revision 1)
>    info("restoring the modified browser state");
>    await TabStateFlusher.flushWindow(window);
>    await promiseBrowserState(SessionStore.getBrowserState());
>    [firstTab, secondTab] = gBrowser.tabs;
>    is(secondTab, gBrowser.selectedTab, "second tab is selected after restoring");
> +  is(document.title.indexOf(CONTENT_TITLE), 0, "title bar displays content title");

nit: same here.

::: browser/components/sessionstore/test/browser_tab_label_during_restore.js:81
(Diff revision 1)
>  
>    info("selecting the first tab");
>    checkLabelChangeCount = observeLabelChanges(firstTab);
>    let tabContentRestored = TestUtils.topicObserved("sessionstore-debug-tab-restored");
>    gBrowser.selectedTab = firstTab;
> +  is(document.title.indexOf(CONTENT_TITLE), 0, "title bar displays content title");

nit: same here.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> > +            aTab._labelIsContentTitle = aOptions && aOptions.isContentTitle;
> 
> I'm kind of starting to dislike the mixed bag and sheer amount of
> properties, attributes and other state-tracking-variables we mix throughout
> the binding and at this rate it's only going to be more.

We could avoid adding this one if sessionstore ensured that browser.contentTitle consistently returned the stored title throughout the restoration process. Do you know if it currently does that? I believe between the lazy, pending, restoring history and loading states, contentTitle may be intermittently empty.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8868961 [details]
Bug 1365780 - Consistently dispatch the TabAttrModified event and update the title bar when setting tab labels.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140630/diff/1-2/

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [::dao] from comment #5)
> We could avoid adding this one if sessionstore ensured that
> browser.contentTitle consistently returned the stored title throughout the
> restoration process. Do you know if it currently does that? I believe
> between the lazy, pending, restoring history and loading states,
> contentTitle may be intermittently empty.

There have been changes there in that regard recently, but it's too early to say 'yes' with 100% certainty. Not with enough certainty, at least, to simplify the patch here.

So please land as-is, thanks.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #7)
> (In reply to Dão Gottwald [::dao] from comment #5)
> > We could avoid adding this one if sessionstore ensured that
> > browser.contentTitle consistently returned the stored title throughout the
> > restoration process. Do you know if it currently does that? I believe
> > between the lazy, pending, restoring history and loading states,
> > contentTitle may be intermittently empty.
> 
> There have been changes there in that regard recently, but it's too early to
> say 'yes' with 100% certainty. Not with enough certainty, at least, to
> simplify the patch here.
> 
> So please land as-is, thanks.

Filed bug 1366246.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90c450a6f8d7
Consistently dispatch the TabAttrModified event and update the title bar when setting tab labels. r=mikedeboer

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/90c450a6f8d7

Comment 11

[Mihai Boldan, Desktop QA [:mboldan]]

I tried to reproduce the issue by manually updating the build to an affected version, but without any success. 
Since the auto-update can be performed only to Latest Nightly, I don't think that the issue can be reproduced.
I confirm that the issue is no longer reproducible if updating to Firefox 56.0a1 (2017-06-22). 
Tests were performed under Mac OS X 10.12.5 and under Windows 10x64.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Mihai Boldan, QA [:mboldan] from comment #11)
> Since the auto-update can be performed only to Latest Nightly, I don't think
> that the issue can be reproduced.

There's no need to auto-update to reproduce the bug. All you need to do is restart Firefox and restore the previous session.

Comment 13

[Mihai Boldan, Desktop QA [:mboldan]]

(In reply to Dão Gottwald [::dao] from comment #12)
> (In reply to Mihai Boldan, QA [:mboldan] from comment #11)
> > Since the auto-update can be performed only to Latest Nightly, I don't think
> > that the issue can be reproduced.
> 
> There's no need to auto-update to reproduce the bug. All you need to do is
> restart Firefox and restore the previous session.

Thanks for the hint.
I managed to reproduce the issue with Firefox Nightly(2017-05-17), under Mac OS X 10.12.
After testing it again, I noticed an inconsistency in Window tab, between a 'new tab' opened in the current session and a 'new tab' opened from the previous session. The new tab opened on the current session appears as 'Nightly / Mozilla Firefox' and the tab opened from the recent session is displayed as 'New Tab' in the Window tab. 
The issue is reproducible also on Firefox 55.b4.
Can you please confirm that this is not an expected behavior? If it's not, I will log a new issue for this problem.
Here is a screenshot of the issue - https://i.imgur.com/gniMQBL.png.

Comment 14

[Dão Gottwald [:dao]]

(In reply to Mihai Boldan, QA [:mboldan] from comment #13)
> (In reply to Dão Gottwald [::dao] from comment #12)
> > (In reply to Mihai Boldan, QA [:mboldan] from comment #11)
> > > Since the auto-update can be performed only to Latest Nightly, I don't think
> > > that the issue can be reproduced.
> > 
> > There's no need to auto-update to reproduce the bug. All you need to do is
> > restart Firefox and restore the previous session.
> 
> Thanks for the hint.
> I managed to reproduce the issue with Firefox Nightly(2017-05-17), under Mac
> OS X 10.12.
> After testing it again, I noticed an inconsistency in Window tab, between a
> 'new tab' opened in the current session and a 'new tab' opened from the
> previous session. The new tab opened on the current session appears as
> 'Nightly / Mozilla Firefox' and the tab opened from the recent session is
> displayed as 'New Tab' in the Window tab. 
> The issue is reproducible also on Firefox 55.b4.
> Can you please confirm that this is not an expected behavior? If it's not, I
> will log a new issue for this problem.
> Here is a screenshot of the issue - https://i.imgur.com/gniMQBL.png.

This is not expected behavior.

Comment 15

[Mihai Boldan, Desktop QA [:mboldan]]

Bug 1377071 was logged for the issue described in Comment 13.