Closed Bug 950132 Opened 6 years ago Closed 6 years ago

Selected stylesheet (page style) not always restored on lazy-loaded tabs

Categories

(Firefox :: Session Restore, defect)

28 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 29
Tracking Status
firefox27 --- unaffected
firefox28 + verified
firefox29 --- verified

People

(Reporter: Felipe, Assigned: ttaubert)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Sometimes, when restoring a Bugzilla tab (I have restore_on_demand = true, haven't tried with false), the selected page style is not properly restored, and the website looks different. It doesn't look like the "No style" version, it just looks like some styles are missing.  Looking at View -> Page Style, no entries in the menu are checked.

It can be easily spotted in Bugzilla, but I've seen it happening on other websites too. It doesn't happen all the time though, but it does happen pretty often. Specially after a restart to update Nightly.

I don't know if this is a SS bug or somewhere else, filing here as it was my first guess. Maybe a regression from bug 910668?
Rather a regression from bug 930967. We should try to find STR for this.
This also happens every now and then when I use Ctrl/Cmd+Shift+T to reopen the last closed tab.
Blocks: 930967
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All
Version: Trunk → 28 Branch
Having STR and confirming the regression range would be great.
Ioana, please have your team attempt to reproduce this bug and provide a regression range if possible.
Flags: needinfo?(ioana.budnar)
STR:
  - configure firefox to restore previous session on startup
  - open bugzilla in multiple tabs in multiple windows (i have 25 tabs across 10 windows)
  - quit then relaunch firefox
  - click on each tab

i always have at least one tab fail. after my last test, 3 failed.


something that may be of interest is the tab is rendered correctly when it first loads, then the stylesheet is dropped (ie. i see a flash of the mozilla skin before it is removed).
(In reply to Byron Jones ‹:glob› from comment #5)
> something that may be of interest is the tab is rendered correctly when it
> first loads, then the stylesheet is dropped (ie. i see a flash of the
> mozilla skin before it is removed).

So does this mean you have the old Bugzilla skin and thus a custom theme?
I've seen this too; I use the "Dusk" theme.
Attached image 950132.gif
(In reply to Tim Taubert [:ttaubert] from comment #6)
> So does this mean you have the old Bugzilla skin and thus a custom theme?

not entirely sure what you're asking :)  i'm using the mozilla skin.

i've attached a gif which clearly shows it initially loading correctly (with the mozilla skin), then changing once loaded.
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #4)
> Ioana, please have your team attempt to reproduce this bug and provide a
> regression range if possible.

An investigation on this matter is already underway. I will post the results later today.
Flags: needinfo?(ioana.budnar)
QA Contact: cornel.ionce
Regression range:

Last Good Nightly: 2013-11-23
First Bad Nightly: 2013-11-24

Pushlog: 
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2013-11-23&enddate=2013-11-24 

I've managed to reproduce this by setting firefox to restore previous session on startup and using 2 windows with 12 bugzilla tabs opened as it follows:
- first window with 5 tabs
- second window with 7 tabs

After closing and reopening firefox at least one tab from each window failed.

It also reproduces using a single window with several tabs, but intermittent.
Selecting the "Dusk" theme I can at least somehow reproduce the issue. For tabs that are restored successfully, document.selectedStyleSheetSet == "Dusk". Whereas for failing tabs (with the lighter background) document.selectedStyleSheetSet == "".
PageStyle.restore() receives "Dusk" but sometimes it receives "Mozilla". The latter doesn't exist and that's why we disable stylesheets, I guess.
So one thing I found is that we don't reset the current pageStyle after a new page has loaded. This is easy to fix once bug 921942 lands. I had some pageStyle collectionoptimizations in mind anyway.

I could only reproduce this by having a few bugzilla tabs and then switching to the Dusk theme. Some tabs didn't get reset and tried to load the "Mozilla" theme next time. Other than that I can't seem to reproduce the failure.
This patch fixes a few things:

1) It resets pageStyle data as soon as a new page is loading. This matches the scroll position behavior as we only want to store that for the currently visible frames.

2) It uses the new FrameTree to only cycle reachable frame trees.

3) It stores data per frame, as that is what we should do. Each document can have its own styleSheetSet.

4) Data collection per frame is now done according to http://dev.w3.org/csswg/cssom/#persisting-the-selected-css-style-sheet-set
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8349979 - Flags: review?(dteller)
Oops, forgot the FrameTree.contains() check.
Attachment #8349979 - Attachment is obsolete: true
Attachment #8349979 - Flags: review?(dteller)
Attachment #8349986 - Flags: review?(dteller)
Comment on attachment 8349986 [details] [diff] [review]
0001-Bug-950132-Fix-pageStyle-data-collection-r-yoric.patch (v2)

Review of attachment 8349986 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/src/PageStyle.jsm
@@ +45,4 @@
>  
> +      if (doc) {
> +        // http://dev.w3.org/csswg/cssom/#persisting-the-selected-css-style-sheet-set
> +        style = doc.selectedStyleSheetSet || doc.lastStyleSheetSet;

Wow, that is impressively more concise.
Why did we go for the regexp + media stuff previously?

@@ +78,5 @@
> +  /**
> +   * Restores pageStyle data for the current frame hierarchy starting at the
> +   * |docShell's| current DOMWindow using the given pageStyle |data|.
> +   *
> +   * If the current frame hierarchy doesn't match that of the given |data|

Nit: I'd add the word "Caveat" or "Warning" somewhere on that line.

@@ +91,5 @@
> +   *          children: [
> +   *            null,
> +   *            {pageStyle: "Mozilla", children: [ ... ]}
> +   *          ]
> +   *        }

Could you document |disabled| in |@param data|?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2987,2 @@
>      ScrollPosition.restoreTree(aBrowser.contentWindow, scrollPositions);
> +    PageStyle.restoreTree(aBrowser.docShell, pageStyle);

Should we really call this if |pageStyle| is a string?

::: browser/components/sessionstore/test/browser_pageStyle_sample.html
@@ +9,5 @@
>    <link href="404.css" title="media_empty" rel="alternate stylesheet" media="">
>    <link href="404.css" title="media_all" rel="alternate stylesheet" media="all">
>    <link href="404.css" title="media_ALL" rel="alternate stylesheet" media=" ALL ">
>    <link href="404.css" title="media_screen" rel="alternate stylesheet" media="screen">
>    <link href="404.css" title="media_print_screen" rel="alternate stylesheet" media="print,screen">

Why did you remove these style sheets?
Attachment #8349986 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach Teller [:Yoric] (away 20 Dec to Jan 4th - please use "needinfo?") from comment #16)
> > +      if (doc) {
> > +        // http://dev.w3.org/csswg/cssom/#persisting-the-selected-css-style-sheet-set
> > +        style = doc.selectedStyleSheetSet || doc.lastStyleSheetSet;
> 
> Wow, that is impressively more concise.
> Why did we go for the regexp + media stuff previously?

I don't know. I guess that when implemented years ago there either wasn't a spec yet or the person didn't know it existed. In any case we did far too much work.

> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +2987,2 @@
> >      ScrollPosition.restoreTree(aBrowser.contentWindow, scrollPositions);
> > +    PageStyle.restoreTree(aBrowser.docShell, pageStyle);
> 
> Should we really call this if |pageStyle| is a string?

No, of course not. Thanks for catching that.

> ::: browser/components/sessionstore/test/browser_pageStyle_sample.html
> @@ +9,5 @@
> >    <link href="404.css" title="media_empty" rel="alternate stylesheet" media="">
> >    <link href="404.css" title="media_all" rel="alternate stylesheet" media="all">
> >    <link href="404.css" title="media_ALL" rel="alternate stylesheet" media=" ALL ">
> >    <link href="404.css" title="media_screen" rel="alternate stylesheet" media="screen">
> >    <link href="404.css" title="media_print_screen" rel="alternate stylesheet" media="print,screen">
> 
> Why did you remove these style sheets?

These styles have been activated artificially by the test. The browser will just accept any string and set that to the selectedStyleSheetSet. It thus doesn't make sense to check for "invalid" sets anymore.
https://hg.mozilla.org/mozilla-central/rev/9f97b68c6bdf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Felipe, can you please check to see if today's Nightly resolves this issue?
Flags: needinfo?(felipc)
The bug was not deterministic, but I haven't been able to see it again using today's Nightly, after trying in a reasonable amount of tabs.

Note that I did at first see it again but I presume it's because there was collected SS data still from the problematic builds. After refreshing all bugzilla tabs that I had and restarting Firefox a number of times, I can't see the bug.

Looks fixed to me. I'll reopen if necessary.
Flags: needinfo?(felipc)
(In reply to :Felipe Gomes from comment #21)
> Looks fixed to me. I'll reopen if necessary.

Thanks
Status: RESOLVED → VERIFIED
Can we get an uplift nomination here if this is low risk enough to take on Aurora?
Flags: needinfo?(ttaubert)
(In reply to Lukas Blakk [:lsblakk] from comment #23)
> Can we get an uplift nomination here if this is low risk enough to take on
> Aurora?

I think that's rather risky unfortunately because it heavily relies on bug 921942 :/
Flags: needinfo?(ttaubert)
What's the alternative?
Flags: needinfo?(ttaubert)
Tiny patch for Aurora.

This basically fixes the core issue here in that we don't restore the pageStyle if that is "" as we would just disable most (or all) of the style sheets.

This didn't happen before because we used to synchronously read the pageStyle whenever we collected tab data. Now that we read the pageStyle only when it's changed while viewing the page we also need to respect null values.
Attachment #8357700 - Flags: review?(dteller)
Flags: needinfo?(ttaubert)
Comment on attachment 8357700 [details] [diff] [review]
0001-Bug-950132-Ignore-empty-pageStyle-data.patch

Review of attachment 8357700 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, although I'm not quite sure why it is sufficient to clear the data
Attachment #8357700 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #27)
> r=me, although I'm not quite sure why it is sufficient to clear the data

I'm not clearing any data, I'm ignoring empty pageStyle data. If we try to set "" as the current pageStyle we disable any style sheet not matching that name.
Sorry, I meant "clear the issue".
Comment on attachment 8357700 [details] [diff] [review]
0001-Bug-950132-Ignore-empty-pageStyle-data.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 930967
User impact if declined: the page style might be restored incorrectly which leads to disabled style sheets and broken styles.
Testing completed (on m-c, etc.): a different patch landed on m-c. I did some smoke testing but this needs to be verified by QA afterwards.
Risk to taking this patch (and alternatives if risky): Low risk.
String or IDL/UUID changes made by this patch: None.
Attachment #8357700 - Flags: approval-mozilla-aurora?
Comment on attachment 8357700 [details] [diff] [review]
0001-Bug-950132-Ignore-empty-pageStyle-data.patch

Thanks Tim, will put verifyme in the whiteboard momentarily.
Attachment #8357700 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Backed out for breaking mochitest-bc. Sorry, I was sure I ran them locally when working on the patch :/

https://hg.mozilla.org/releases/mozilla-aurora/rev/dbe88da38289
Had to correct page style tests that assumed no style sheets were enabled for "fail_" test cases. Instead the default style sheet should stay enabled to not have a page without styles as seen here.

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e9d1f109655
Verified the bug using the following environment:

FF 28 beta
Build Id: 20140203225656
OS: Win 7 x64 , Ubuntu 12.04 x32 , Mac Os x 10.9
You need to log in before you can comment on or make changes to this bug.