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

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

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.

http://hg.mozilla.org/mozilla-central/annotate/cb9f34f73ebe/toolkit/modules/NewTabUtils.jsm#l836

Comment 1

[:Gavin Sharp [email: [e-mail]]]

(I'm testing on today's Nightly build in an old profile)

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Regression of an important feature, tracking.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1013051-NewTabUtils-should-support-legacy-pinned.patch

Comment 5

[Drew Willcoxon :adw]

Attachment: patch

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: http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/NewTabUtils.jsm?rev=cd8697531517#927

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.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8434668 [details] [diff] [review]
patch

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.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Thanks for taking over and writing the patch!

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Marco, can you please re-assign this bug to Drew? He did all the work here. Thanks!

Comment 9

[Marco Mucci [:MarcoM]]

Done

Comment 10

[Drew Willcoxon :adw]

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Drew, could you request the uplift to aurora and beta?

Comment 13

[Drew Willcoxon :adw]

Attachment: Aurora/Beta patch (and the patch that was actually landed on fx-team)

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:
none

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Moderate risk, OK. I will accept the uplift in aurora first and take it for beta 4 then.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9c69f87096d4

Comment 16

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 17

[Drew Willcoxon :adw]

Thanks, Sylvestre.  https://hg.mozilla.org/releases/mozilla-beta/rev/34e4ea875ee2