991210 - [new tab page] Tiles are sometimes arranged all in a single line (wrapping as appropriate, e.g. to two lines with 5 items and then 4 items), instead of 3x3 grid

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: screenshot.png

I don't have STR, but this has happened twice in the past few days, so it's not a fluke.

Basically, occasionally my "new tab" page has the tiles in an awkward 2-row layout:
  1 2 3 4 5
   6 7 8 9

...instead of...
  1 2 3
  4 5 6
  7 8 9

This just happened to me when I quit & reopened Firefox, when a new tab page was restored. Subsequent "new tab" pages that I opened had the correct 3x3 layout.

Screenshot attached

Comment 1

[Daniel Holbert [:dholbert]]

I'm using up-to-date nightly:
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0
31.0a1 (2014-04-02)

(Also: I don't recall the exact circumstances on the previous instance where I hit this; I'm not sure if it was from simply opening a new tab page, or restoring one on startup. As noted in comment 0, though, when I hit this just now, it was from restoring a page on startup.)

Comment 2

[Brian Birtles (:birtles)]

Attachment: screenshot-windows.png

Comment 3

[Brian Birtles (:birtles)]

Seeing this on Windows 7 x64. Likewise, I can't reproduce it consistently but it has happened several times in the last 2~3 days. If I open a new tab at the same time the new tab is laid out in a grid.

Comparing the two tabs I notice that newtab-grid element has an empty inline style in the error case. Screenshots forthcoming.

Comment 4

[Brian Birtles (:birtles)]

Attachment: missing-style.png

Comment 5

[Brian Birtles (:birtles)]

Attachment: with-style.png

Comment 6

[Brian Birtles (:birtles)]

Bug 980014 seems like a likely culprit. Ed, any ideas?

Comment 7

[Ed Lee :Mardak]

This is in the midst of using Firefox normally? Not on first opening of a newtab when restarting Firefox? Or did you open up multiple new tabs in a row?

You do mention seeing this at startup when restoring, but have you run into this any other time?

Comment 8

[Daniel Holbert [:dholbert]]

AFAIK, I've only hit this when restoring (and/or when opening the first tab after restoring).

(I hit this once today after filing, too - so I've seen this at least 3 times now.)

Comment 9

[Ed Lee :Mardak]

(In reply to Brian Birtles (:birtles) from comment #4)
> Created attachment 8401017 [details]
> missing-style.png
Oh? The tiles stayed split across 2 rows even after you opened up the inspector? When you run into the issue, if you resize the full window, does it fix itself? (I would guess no because opening the inspector should have triggered the resize event...)

Comment 10

[Daniel Holbert [:dholbert]]

I'm pretty sure I tried resizing the window, and it did not fix things.

Comment 11

[Brian Birtles (:birtles)]

(In reply to Ed Lee :Mardak from comment #9)
> Oh? The tiles stayed split across 2 rows even after you opened up the
> inspector? When you run into the issue, if you resize the full window, does
> it fix itself? (I would guess no because opening the inspector should have
> triggered the resize event...)

I've closed that tab now so I can't reproduce. I'll let you know if I reproduce it again.

Also, I don't recall restoring when I saw this happen but does loading pinned tabs count as restoring?

Comment 12

[Marco Bonardo [:mak]]

sounds like the same as bug 989500, even if there the issue seems to be related to backing out promises changes... I wonder if it's in reality due to a mix of the two, if I read times correctly Bug 980014 landed while bug 895359 was in-tree, and just after that bug 895359 was backed out.

Comment 13

[Daniel Holbert [:dholbert]]

I'm playing with this in a tab that I just opened to reproduce this. This was not at startup; I just opened a new tab and it hit this.

I noticed that if I make the window super-wide, the items end up all on a single line. They just behave like inline elements, line-wrapping into the window-size, with each line getting centered.

Comment 14

[Ed Lee :Mardak]

dholbert was able to test some stuff out for me with a live broken newtab.

The resize event handler is running, but..

console.log(gGrid._cellMargin, gGrid._cellHeight, gGrid._cellWidth)

all return undefined.

Those are normally set on "load," so either load never happened or it happened at some time that didn't allow for computing _cellMargin, etc.

Comment 15

[Drew Willcoxon :adw]

It looks like it's possible for gGrid's load listener to be added after the page has loaded, since it's added after gLinks.populateCache finishes (via gPage._init), which could be any time.  So maybe gGrid.init should check document.readyState?

Comment 16

[Ed Lee :Mardak]

Is anyone able to consistently reproduce this issue? I was seeing it on Mac Nightly with "show my windows and tabs from last time" with about:newtab being the first tab loaded on restart. Except I updated Nightly and couldn't reproduce the problem.

If it's still consistently broken on Windows on restart, could you try:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-70cab4cc2935/try-win32/
https://tbpl.mozilla.org/?tree=Try&rev=70cab4cc2935

Comment 17

[Daniel Holbert [:dholbert]]

(I've never been able to consistently reproduce this [on Linux]; I've only hit it at most once or twice a day.)

Comment 18

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #15)
> It looks like it's possible for gGrid's load listener to be added after the
> page has loaded, since it's added after gLinks.populateCache finishes
Thanks for the insight. The bug should be fixed https://hg.mozilla.org/try/rev/26e0ba79a5cf

maksik is writing a test where..

turn off preload
AfterLoadProvider = {
  getLinks: function(callback) { this.callback = callback; }
};
Links.addProvider(AfterLoadProvider);
open new tab page
newtabpage.addEventListener(“load”, AfterLoadProvider.callback())

This forces gLinks.populateCache to trigger its callback after the page has loaded.

Comment 19

[u279076]

Flagging for QA testing.

Comment 20

[Ed Lee :Mardak]

Attachment: v1

Thanks maksik for the test

Comment 21

[Drew Willcoxon :adw]

Comment on attachment 8404345 [details] [diff] [review]
v1

I think the test is vulnerable to the same problem as this bug because the PlacesProvider may finish getting its links *after* the afterLoadProvider calls its callback.  In that case, the gLinks.populateCache call in gPage._init still would not have finished, so gGrid.init would not have been called when the test goes on to check _cellMargin.

I'm starting to think that there's no reason for gPage._init to wait until populateCache is done to build the grid.  Rendering the sites in the grid is a different story and must wait until all links have been populated of course.  You could argue that it's bad UX to have the sites visually pop into the grid if for whatever reason populateCache takes a noticeable while, but I'd argue that's no worse than having the entire grid pop in -- although with the latter it would be easier to fade the entire thing in.  And consider the search bar.  Its width depends on the grid's width, but it's always present in the page because it's in the XUL.  Right now there's a window of time where the search bar is not the right width because the grid hasn't been created because populateCache hasn't finished.  Creating the grid immediately would fix that.  But so would deferring creating the search bar.

So the test either needs to wait until gGrid.init or _resizeGrid is called (probably easiest by replacing either with a shim method that calls the real one), or we change the grid creation behavior.

What do people think?

Comment 22

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #21)
> I think the test is vulnerable to the same problem as this bug because the
> PlacesProvider may finish getting its links *after* the afterLoadProvider
> calls its callback.
Ah yes, the test should be continuing at the same time populateCache would have finished. Similar to addNewTabPageTab in the test does:
NewTabUtils.links.populateCache(() => executeSoon(TestRunner.next));

> I'm starting to think that there's no reason for gPage._init to wait until
> populateCache is done to build the grid.
So from Page_init, instead of calling gGrid.init inside gLinks.populateCache, it should call gGrid.renderSites (removed _ from _renderSites).

Comment 23

[Ed Lee :Mardak]

Let's try this:
https://hg.mozilla.org/try/rev/fe45fab739aa
https://tbpl.mozilla.org/?tree=Try&rev=da0de201dde7

I removed the populateCache call from Page._init and moved it into Grid.init. I also unindented the things inside populateCache which seems to be undoing some of bug 721413 https://hg.mozilla.org/mozilla-central/rev/d0b8df7e28ea but that bug was more to get rid of the eager populateCache call from NewTabUtils.init.

Comment 24

[Jesper Hansen]

Going to cc on here and add the comment that 4x4 tiles are showing up as 4x3 instead now.
Could be that the patch hasn't hit the nightly build just yet.

Comment 25

[Elbart]

Does this bug also cover the wrong behavior of zooming _out_ of the newtab-page (Ctrl+ MouseWheel down) narrowing down the grid to 2x3 tiles with the default settings?

Comment 26

[Daniel Holbert [:dholbert]]

Nope, that looks like an independent issue. Not sure if that's already tracked in another bug, but if it isn't, it should be. (I can reproduce, FWIW.)

Comment 27

[Ed Lee :Mardak]

Attachment: v2

Move the delayed-until-after-populateCache calls to the same level as where populateCache would have been, and move the call into Grid_init to only delay rendering of sites.

https://tbpl.mozilla.org/?tree=Try&rev=1d2154554d33

Comment 28

[Drew Willcoxon :adw]

Comment on attachment 8404963 [details] [diff] [review]
v2

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

Thanks, Ed.  This is what I had in mind, but afterLoadProvider isn't necessary, is it?  It doesn't necessarily delay populateCache from finishing (due to PlacesProvider), but on the other hand it doesn't have to, because now the grid is resized as soon as Grid_init is called, which is before page load even.  So all the test really needs to do is wait for load, and the _cell* properties should be ready.

I think the test also ought to check that the grid was actually resized, i.e., that _node.style.height, etc. are what they should be according to Grid_resizeGrid.

Comment 29

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #28)
> afterLoadProvider isn't necessary, is it?
It forces the test to always fail without the patch and passes with the patch by making sure populateCache only finishes after load. True, PlacesProvider could finish after load as well to cause the bug, but afterLoadProvider ensures the bug will happen (without patch).

> now the grid is resized as soon as Grid_init is called
Ah yes, and I think that's wrong (in that it unnecessarily calls resizeGrid that always returns early because readyState != complete, so it doesn't actually resize as you might have thought).

There are 3 events:
1) before page load (loading)
2) on page load (complete)
3) populateCache callback

For (1), the html is created for the grid to be rendered while (2) has the page rendered with sizes to be read out to resize the grid then (3) populates the grid with sites.

This bug happens when (3) happens before (2), because Grid_init used to add the load listener after the page is already complete. The fix should just always call Grid_init -> adds load listener /before/ (2). Attachment 8404963 [details] [diff] unnecessarily calls resizeGrid from _render and checks readyState when actually we don't even need to check readyState if we only call resizeGrid from load or later.

> I think the test also ought to check that the grid was actually resized,
Mmm... how would you do this without copying the same logic implemented in grid.js?

Comment 30

[Ed Lee :Mardak]

Attachment: v3

Implements the cleanup per comment 29 by inlining Grid_render into Grid_init (simplifying in the process) and into Grid_refresh while also removing the explicit readyState check in Grid_resizeGrid as it won't be called before load.

Although this attachment caused browser_newtab_unpin test failures 3 out of 10 osx 10.6 debug bc1 https://tbpl.mozilla.org/?tree=Try&rev=3d0143307759
But the previous attachment 8404963 [details] [diff] [review] also caused 2 out of 10 of the same test failures https://tbpl.mozilla.org/?tree=Try&rev=8c714cb4dc02

Got 1,2,2,3,4,5,6,7,, expected 0,1,2,3,4,5,6,7,

That's the failure for these lines:
  checkGrid("1,2,3,4,5,6,7,,0p");
  yield unpinCell(8);
  checkGrid("0,1,2,3,4,5,6,7,");

Any idea what would cause the missing 0 and duplicate 2?

Comment 31

[Ed Lee :Mardak]

(In reply to Ed Lee :Mardak from comment #30)
> Got 1,2,2,3,4,5,6,7,, expected 0,1,2,3,4,5,6,7,
> Any idea what would cause the missing 0 and duplicate 2?
Actually, somehow there's 10 items in the current state..?
0: got 1 expect 0
1: got 2 expect 1
2: got 2 expect 2
3: got 3 expect 3
4: got 4 expect 4
5: got 5 expect 5
6: got 6 expect 6
7: got 7 expect 7
8: got "" expect ""
9: got "" expect nothing

checkGrid slices the sites from 0 to 9, so somehow an extra item is getting in...

Comment 32

[Ed Lee :Mardak]

Attachment: v4

Turns out the test was sometimes failing because a previous test happened to start _scheduleUpdateTimeout that triggered during another test. And because Grid_init is called right away, it was getting its _node set making it seem like it was .ready for Page_update -> Grid_refresh.

This patch 1) clears any outstanding timers and 2) only sets Grid.ready after the sites have been rendered.

Comment 33

[Ed Lee :Mardak]

(In reply to Ed Lee :Mardak from comment #32)
> This patch 1) clears any outstanding timers and 2) only sets Grid.ready
> after the sites have been rendered.
I tested with one, the other, and both fixes and they all seem to pass tests:
both: https://tbpl.mozilla.org/?tree=Try&rev=3293042a1eca
clear timeout: https://tbpl.mozilla.org/?tree=Try&rev=830363bf1126
delay ready: https://tbpl.mozilla.org/?tree=Try&rev=030f2b7537d6

Comment 34

[Drew Willcoxon :adw]

Comment on attachment 8406691 [details] [diff] [review]
v4

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

Thanks, Ed.  Stopping _scheduleUpdateTimeout in head.js is a great idea.  That should remove the need for the AllPages.updateScheduledForHiddenPages getter and its use in browser_newtab_update.js.  Would you mind removing those two things as part of this patch?  If you'd prefer not to, I understand.

(In reply to Ed Lee :Mardak from comment #29)
> > I think the test also ought to check that the grid was actually resized,
> Mmm... how would you do this without copying the same logic implemented in
> grid.js?

Yeah, that's what I had in mind, just to catch possible regressions in the future when we make small changes to grid.js without intending to change the overall design.  If you don't think it's necessary, that's fine.

Comment 35

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/cd8697531517

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd8697531517

Comment 37

[Ed Lee :Mardak]

I'm getting Tp5 regressions (Optimized Responsiveness, Main RSS, Private Bytes) across platforms roughly 6-9% with the exception of 17% Tp5 Optimized Responsiveness - MacOSX 10.6 (rev4) - 17%

The push contained this bug 991210 and bug 991111, but if it's either bug, most likely it's this one.

http://mzl.la/1gGtw7d Tp5 Optimized Responsiveness - WINNT 5.1 (ix) - 7.27%
http://mzl.la/1gGtyfo Tp5 Optimized (Main RSS) - WINNT 5.1 (ix) - 5.93%
http://mzl.la/1gGtyMo Tp5 Optimized (Private Bytes) - WINNT 5.1 (ix) - 6%

Anyone have any ideas? I'm guessing it's because we cause the grid html to be created sooner instead of deferring it until populateCache finishes.

Comment 38

[Ed Lee :Mardak]

(In reply to Ed Lee :Mardak from comment #37)
> The push contained this bug 991210 and bug 991111, but if it's either bug,
> most likely it's this one.
How wrong I was. It was actually bug 991111. Fixing in bug 998163.

Comment 39

[Ed Lee :Mardak]

For those who ran into this before, please report back if you see it happen after updating to 31.0a1 (2014-04-17)+

Comment 40

[Cornel Ionce [:noni] [Hubs QA]]

Randomly reproduced this issue using 2014-04-02 Nightly.
It did not reproduce using latest Nightly (build ID: 20140418030202).

Being an intermittent issue it will be quite difficult to see if it's actually fixed.

Daniel, have you ran into this lately?

Comment 41

[Daniel Holbert [:dholbert]]

I have not, but I'll keep an eye out for it.

Comment 42

[Jared Wein [:jaws] (please needinfo? me)]

Previously I could reproduce it by doing a restarting the browser (via applying a Nightly update) when I had a New Tab Page open. With the 4/18 Nightly I can't reproduce anymore (tested with the Nightly update as well as the Restartless Restart add-on).