Closed Bug 1013051 Opened 6 years ago Closed 6 years ago

"Comparable link missing required property: frecency" error showing up in console constantly


(Firefox :: New Tab Page, defect)

Not set



Firefox 33
Tracking Status
firefox29 --- unaffected
firefox30 --- unaffected
firefox31 + fixed
firefox32 + fixed
firefox33 --- fixed


(Reporter: Gavin, Assigned: adw)



(Keywords: regression, Whiteboard: [tiles] p=3 s=33.1 [qa-])


(2 files, 1 obsolete file)

I opened the JS console to debug a web page and saw a bunch of these. Leaving the console window open and doing nothing, I keep getting more at somewhat random intervals.
Flags: firefox-backlog+
(I'm testing on today's Nightly build in an old profile)
I have the same in my console for a few days or more now. I assume it has to do with my old list of pinned sites on the new tab page that don't have any of the new fancy properties?
Regression of an important feature, tracking.
Whiteboard: p=0 [qa?]
Assignee: nobody → ttaubert
Whiteboard: p=0 [qa?] → p=3 s=it-32c-31a-30b.3 [qa-]
Attached patch patchSplinter Review
I can reproduce this, too.  The problem isn't pinned links, and this exception really does indicate a logic error, so it's not really right to just conditionally check for sort properties.  (Pinned links don't participate in sorting and are merged in at the last minute in Links.getLinks.)

The failing compareLinks call is this one:

That block of code assumes that if the link is not in the linkMap, then it's a new link and therefore has all the required sort properties.  But the provider may have previously notified Links about the link -- it's an "old" link -- and Links may have ignored it because of exactly what that block is checking, because the list is full and the link isn't ranked highly enough to be in it.

So if the link is such an old link, and it's not in the list, and the provider has called onLinkChanged because the link's title has changed, then the link doesn't have all the sort properties, and the compareLinks call fails.

This patch reworks Links.onLinkChanged and fixes the problem by making sure links that are not in the list have all the sort properties before trying to compare them or add them to the list.  It also fixes another unrelated problem where onLinkChanged was modifying object properties before it looked up that object in the list.
Attachment #8434668 - Flags: review?(ttaubert)
Attachment #8433259 - Flags: review?(adw)
Whiteboard: p=3 s=it-32c-31a-30b.3 [qa-] → p=3 s=33.1 [qa-]
Attachment #8433259 - Attachment is obsolete: true
Comment on attachment 8434668 [details] [diff] [review]

Review of attachment 8434668 [details] [diff] [review]:

::: toolkit/modules/NewTabUtils.jsm
@@ +957,5 @@
> +      // Copy the link object so that changes later made to it by the caller
> +      // don't affect our copy.
> +      insertionLink = {};
> +      for (let prop in aLink) {
> +        insertionLink[prop] = aLink[prop];

I guess you could use Cu.cloneInto() but it doesn't make a big difference.
Attachment #8434668 - Flags: review?(ttaubert) → review+
Thanks for taking over and writing the patch!
Assignee: ttaubert → adw
Marco, can you please re-assign this bug to Drew? He did all the work here. Thanks!
Flags: needinfo?(mmucci)
Flags: needinfo?(mmucci)
Component: General → New Tab Page
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Drew, could you request the uplift to aurora and beta?
Flags: needinfo?(adw)
This is the patch that landed on fx-team and m-c.  It improves a couple of code comments over the patch that was r+'ed here in the bug.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
bug 911307

User impact if declined:
Users will see frequent errors in the browser console.

Testing completed (on m-c, etc.):
automated testing on m-c

Risk to taking this patch (and alternatives if risky):
Moderate risk because this modifies about:newtab code.  If there's a bug in this patch, it could affect the choice of sites that are shown to the user in about:newtab, or the ordering of those sites.  There's less risk that this could break about:newtab in a more significant way.

String or IDL/UUID changes made by this patch:
Attachment #8440794 - Flags: approval-mozilla-beta?
Attachment #8440794 - Flags: approval-mozilla-aurora?
Flags: needinfo?(adw)
Moderate risk, OK. I will accept the uplift in aurora first and take it for beta 4 then.
Attachment #8440794 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8440794 [details] [diff] [review]
Aurora/Beta patch (and the patch that was actually landed on fx-team)

Beta 3 is built. Taking it for beta4.
Attachment #8440794 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: p=3 s=33.1 [qa-] → [tiles] p=3 s=33.1 [qa-]
You need to log in before you can comment on or make changes to this bug.