Closed Bug 1067173 Opened 5 years ago Closed 5 years ago

The page with Tiles is not rendered correctly

Categories

(Firefox :: New Tab Page, defect)

33 Branch
x86
macOS
defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 35
Iteration:
36.1
Tracking Status
firefox32 --- unaffected
firefox33 + verified
firefox34 + verified
firefox35 --- verified

People

(Reporter: mkem, Assigned: ttaubert)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached image correct.png
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20140911191954

Steps to reproduce:

1. we have to have set up home page to "about:newtab"
2. open page www.abclinuxu.cz or www.slovnik.cz or speedtest.net
3. click on home icon in toolbar menu
4. new page with tiles is shown, but it is not rendered properly. I attached the screenshot with correct rendering and incorrect rendering


Actual results:

After the steps described upper the page with Tiles is not rendered correctly. Strange is that it is not happened with every page. I was able to reproduce it with pages meant upper. Maybe it has something together with that previews of problematic pages are also in tiles.  


Expected results:

Every time correct rendering of page with Tiles
Attached image corrupted.png
Additional info: If I have incorrect rendered page with Tiles and change the size of window, nothing happens. I also made "reset Firefox to its default state". But the results are the same.
I just tested the problem in Firefox 32.0.1 and the problem with rendering of page with Tiles didn't happen.
When you get such a broken new tab page, if you open the browser console (Tools > Web Developer > Browser Console) are there any errors that seem relevant?
Severity: normal → critical
Component: Untriaged → New Tab Page
Flags: needinfo?(mkem)
I checked via the console and find only this message: 
Error during parsing value "max-height". Declaration was omitted.

Nothing else. When I have broken newtab page and click again on icon "home". I get the newtab page correctly rendered.
Flags: needinfo?(mkem)
Do you have any custom values for browser.newtab* in particular browser.newtab.preload ?
All properties of browser.newtab* are set to default. Only these ones are set to the following values. 

browser.newtabpage.enhanced;true
browser.newtabpage.storageVersion;1
Severity: critical → normal
I can't reproduce the issue with the given STR but was able to with some fiddling around in the code. My guess is that we somehow end up calling _resizeGrid() earlier than expected, before the document has finished loading. This leads to a negative max-height value and yields the given error message. Should be safe to just bail out in that case and wait for the load event to call _resizeGrid() again.
Assignee: nobody → ttaubert
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8492663 - Flags: review?(edilee)
Comment on attachment 8492663 [details] [diff] [review]
0002-Bug-1067173-Bail-out-early-if-_resizeGrid-is-called-.patch

>diff --git a/browser/base/content/newtab/grid.js b/browser/base/content/newtab/grid.js
>+++ b/browser/base/content/newtab/grid.js
>   _resizeGrid: function Grid_resizeGrid() {
>+    if (document.readyState != "complete") {
>+      return;
The codepaths triggering _refreshGrid should be from:
1) Page_update -> Grid_refresh
2) Grid_handleEvent(load)
3) Grid_handleEvent(resize)

It shouldn't be (2) because that should only happen when document.readyState == "complete" so something is triggering an update each time or the window is being resized? Either way, we don't want to try to read out sizes until things have flowed.

I can't give r+, but looks good to me!
Attachment #8492663 - Flags: review?(edilee)
Attachment #8492663 - Flags: review?(adw)
Attachment #8492663 - Flags: feedback+
Comment on attachment 8492663 [details] [diff] [review]
0002-Bug-1067173-Bail-out-early-if-_resizeGrid-is-called-.patch

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

This is a worthwhile precaution, but since it's not clear what's causing the bug I'm not convinced this will fix it -- but probably none of us are. :-)  Only thing is that this might end up masking some deeper problem.

Of Ed's three code paths, (1) seems most likely to me, like browser history changes, triggering an update for hidden pages, and then a newtab is preloaded but before it finishes loading its update() is called.  Maybe gAllPages.register(this) in gPage.init should be moved to a load listener?
Attachment #8492663 - Flags: review?(adw) → review+
(In reply to Drew Willcoxon :adw from comment #10)
> This is a worthwhile precaution, but since it's not clear what's causing the
> bug I'm not convinced this will fix it -- but probably none of us are. :-)

Indeed.

> Only thing is that this might end up masking some deeper problem.

I'm not too worried about that... I will land it and keep thinking about a better approach to this. We can also talk about this in person next week and see if we can come up with something nicer. In any case we'd have a small patch we could easily uplift to 33?

> Of Ed's three code paths, (1) seems most likely to me, like browser history
> changes, triggering an update for hidden pages, and then a newtab is
> preloaded but before it finishes loading its update() is called.

Page.upate() could call it when populateCache() has already finished, yeah. The "resize" event handler could probably also be called when we don't expect it.

> Maybe gAllPages.register(this) in gPage.init should be moved to a load listener?

I'd rather not do this as it would defer rendering the page a bit. But let's talk next week?
[Tracking Requested - why for this release]:

This issue has been reported for Firefox 33 but isn't easily reproducible unfortunately. It very visibly breaks the new tab page - recovering seems rather easy but I assume that's only true for the more experienced users.
https://hg.mozilla.org/mozilla-central/rev/d8cdf28a2eb4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Tim, could you fill the uplift requests? Thanks
Flags: needinfo?(ttaubert)
Sure, just wanted to wait for any possible regression reports. Haven't seen any.
Flags: needinfo?(ttaubert)
Comment on attachment 8492663 [details] [diff] [review]
0002-Bug-1067173-Bail-out-early-if-_resizeGrid-is-called-.patch

Approval Request Comment
[Feature/regressing bug #]: ?
[User impact if declined]: Newtab page can break visibly, showing only a single cut-off tile. Recovery possible but may leave some users clueless.
[Describe test coverage new/current, TBPL]: No additional test because the original issue is hard to reproduce.
[Risks and why]: Risk seems low, we will simply not attempt to resize the grid if the page isn't ready yet.
[String/UUID change made/needed]: None
Attachment #8492663 - Flags: approval-mozilla-beta?
Attachment #8492663 - Flags: approval-mozilla-aurora?
Attachment #8492663 - Flags: approval-mozilla-beta?
Attachment #8492663 - Flags: approval-mozilla-beta+
Attachment #8492663 - Flags: approval-mozilla-aurora?
Attachment #8492663 - Flags: approval-mozilla-aurora+
Blocks: 1050643
Michal, can you please confirm whether this is fixed in:
- Firefox 34 Beta 1 - ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/34.0b1-candidates/build2/
or
- latest Firefox Aurora - ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora/
or
- latest Firefox Nightly - ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/

Thank you in advance!
Flags: needinfo?(mkem)
Hello, the bug disappeared in the last 2 betas of Firefox 33. Now I'm using Firefox 34b1 and it also looks great and works without any flaw. 
Just for assurance I tested the versions, you posted in your comment, and no issue appeared. Everything works flawless. The posted versions were tested on OS X 10.10 fresh installed. 
Thank you for your attention and quick fix of this bug. Michal.
Flags: needinfo?(mkem)
Verified by reporter per comment 20. Thanks Michal!
Status: RESOLVED → VERIFIED
Flags: firefox-backlog+
Iteration: --- → 36.1
Flags: qe-verify+
Points: --- → 3
You need to log in before you can comment on or make changes to this bug.