Closed Bug 1365780 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

Unspecified
All
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 + verified
firefox56 --- verified

People

(Reporter: jryans, Assigned: dao)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

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.
[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 :(
Blocks: 1364127
Flags: needinfo?(past)
Flags: needinfo?(dao+bmo)
OS: Unspecified → All
(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.
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Component: Session Restore → Tabbed Browser
Flags: needinfo?(dao+bmo)
Flags: needinfo?(past)
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.
Attachment #8868961 - Flags: review?(mdeboer) → review+
(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.
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(mdeboer)
(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.
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/90c450a6f8d7
Consistently dispatch the TabAttrModified event and update the title bar when setting tab labels. r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/90c450a6f8d7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
(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.
(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.
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
Bug 1377071 was logged for the issue described in Comment 13.
You need to log in before you can comment on or make changes to this bug.