Closed Bug 1059591 Opened 5 years ago Closed 5 years ago

Incorrectly formatted remotely hosted links causes new tab to be empty

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 35
Iteration:
35.2
Tracking Status
firefox33 + verified
firefox34 + verified
firefox35 --- verified

People

(Reporter: Mardak, Assigned: Mardak)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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.
Resolution: FIXED → INVALID
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.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Duplicate of this bug: 1059979
Depends on: 975211
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).
Sorry, comment 5 isn't enough. After resetting the pref, restart the browser.
Duplicate of this bug: 1060026
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.
Status: REOPENED → NEW
[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.
Assignee: nobody → edilee
Depends on: 1042876
Flags: firefox-backlog+
Flags: qe-verify?
Attached patch v1Splinter Review
Attachment #8494083 - Flags: review?(adw)
It would be nice if a fix could land soon.
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.
Attachment #8494083 - Flags: review?(adw) → review+
https://hg.mozilla.org/integration/fx-team/rev/dea5ec3d6ef9
Blocks: 1050643
Points: --- → 3
Target Milestone: --- → Firefox 35
Should this actually be put on the 35.2 iteration?
Flags: qe-verify? → qe-verify+
QA Contact: cornel.ionce
Blocks: 1072706
When it is resolved it will be.

(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #15)
> Should this actually be put on the 35.2 iteration?
https://hg.mozilla.org/mozilla-central/rev/dea5ec3d6ef9
Status: NEW → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Iteration: --- → 35.2
Ed, could you ask for an uplift to aurora and beta? Thanks
Flags: needinfo?(edilee)
Attached patch for upliftSplinter Review
Flags: needinfo?(edilee)
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
Attachment #8496360 - Flags: approval-mozilla-beta?
Attachment #8496360 - Flags: approval-mozilla-aurora?
Attachment #8496360 - Flags: approval-mozilla-beta?
Attachment #8496360 - Flags: approval-mozilla-beta+
Attachment #8496360 - Flags: approval-mozilla-aurora?
Attachment #8496360 - Flags: approval-mozilla-aurora+
Flags: in-testsuite+
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
Also verified on Firefox 33 beta 8, build ID: 20140929180120
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.