1059591 - Incorrectly formatted remotely hosted links causes new tab to be empty

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

My new tab is showing all tiles with empty tiles.

$ curl -L -d '{"locale":"en-US"}' https://tiles.up.mozillalabs.com/v2/links/fetch
{"en-US": {"en-US": [{"directoryId": 1, ..

There's an extra layer of { "en-US", so when the client accesses "en-US" and expects an array but gets an object, .map fails:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/DirectoryLinksProvider.jsm#382
382       aCallback(rawLinks.map((link, position) => {

rawLinks.map fails and the callback is never called, so newtab page is borked.

Comment 1

[Olivier Yiptong [:oyiptong]]

It was an error on my part. I was using a WIP tool for generating the new tile's ID.

A correct payload has been uploaded and deployed to the CDN, so the problem should be fixed now.

Comment 2

[Ed Lee :Mardak]

For the record, the hosted file has been updated to correctly only have one level of "en-US", so there's only one object with an array inside. Instead of an object with an object with an array.

This file was available for ~3 hours, so users who happened to get that file will see empty new tab tiles for 24 hours before the client fetches the correct file.

Comment 3

[Ed Lee :Mardak]

Reopening: The client should probably be more resilient to invalid data file -- at least not break the rest of new tab UI/functionality.

I don't think we'll need to uplift this assuming we're "good" going forwards in terms of serving valid payloads.

Comment 5

[Ed Lee :Mardak]

For those who are running into this bug, you can go to about:config and change the value for the preference "browser.newtabpage.directory.source" to "", then reset the preference (right-click -> reset).

Comment 6

[Ed Lee :Mardak]

Sorry, comment 5 isn't enough. After resetting the pref, restart the browser.

Comment 8

[Ed Lee :Mardak]

Some notes on what's going on:

NewTabUtils.populateCache triggers getLinks for each provider and decrements a counter until all providers return. In the meantime, future populateCache calls (happens for each new tab page) get queued because NewTabUtils remembers that it has asked the providers for data.

But DirectoryLinksProvider.getLinks never calls its callback (which decrements the counter) because remotely fetched data has an {object} instead of [array], so .map throws an exception.

Setting the directory.source pref forces a download of the correct remotely hosted file, but a restart is required because NewTabUtils will wait forever for its counter to decrement to 0.

Comment 9

[Ed Lee :Mardak]

[Tracking Requested - why for this release]: Now that we're using live links data with bug 1042876, it would be safer for users' Firefox to not have the possibility of being broken for at least 24 hours if somehow our servers returned the wrong data.

Comment 10

[Ed Lee :Mardak]

Attachment: v1

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Tracking (cf comment #9)

Comment 12

[Sylvestre Ledru [:Sylvestre]]

It would be nice if a fix could land soon.

Comment 13

[Drew Willcoxon :adw]

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

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

Great, thanks.  As follow-up work, maybe getLinks should return a promise instead of taking a callback to make it more likely errors will be caught.  Links could then handle error recovery for all types of providers.

Comment 14

[Ed Lee :Mardak]

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

Comment 15

[Robert Kaiser]

Should this actually be put on the 35.2 iteration?

Comment 16

[Marco Mucci [:MarcoM]]

When it is resolved it will be.

(In reply to Robert Kaiser (:[e-mail]) from comment #15)
> Should this actually be put on the 35.2 iteration?

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Ed, could you ask for an uplift to aurora and beta? Thanks

Comment 19

[Ed Lee :Mardak]

Attachment: for uplift

Comment 20

[Ed Lee :Mardak]

Comment on attachment 8496360 [details] [diff] [review]
for uplift

Approval Request Comment
[Feature/regressing bug #]: Bug 1030832 / Enhanced Tiles
[User impact if declined]: Potential for new tab page to be empty and broken for 24 hours if the server provides a wrong data file
[Describe test coverage new/current, TBPL]: Test added for this bug
[Risks and why]: Low - minor code changes adding exception handling
[String/UUID change made/needed]: None

Comment 21

[Ed Lee :Mardak]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cf719ba91118

status-firefox33 wontfix per bug 1073823

Comment 22

[Ed Lee :Mardak]

https://hg.mozilla.org/releases/mozilla-beta/rev/d34488e06177

Comment 23

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

Verified this issue on Windows 7 64-bit, Ubuntu 12.04 and Mac OS X 10.9 using:
- latest Nightly, build ID: 20140928030206
- latest Aurora, build ID: 20140928004003

Comment 24

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

Also verified on Firefox 33 beta 8, build ID: 20140929180120