Closed
Bug 1013051
Opened 11 years ago
Closed 10 years ago
"Comparable link missing required property: frecency" error showing up in console constantly
Categories
(Firefox :: New Tab Page, defect)
Firefox
New Tab Page
Tracking
()
VERIFIED
FIXED
Firefox 33
Tracking | Status | |
---|---|---|
firefox29 | --- | unaffected |
firefox30 | --- | unaffected |
firefox31 | + | fixed |
firefox32 | + | fixed |
firefox33 | --- | fixed |
People
(Reporter: Gavin, Assigned: adw)
References
Details
(Keywords: regression, Whiteboard: [tiles] p=3 s=33.1 [qa-])
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Flags: firefox-backlog+
Reporter | ||
Comment 1•11 years ago
|
||
(I'm testing on today's Nightly build in an old profile)
Comment 2•11 years ago
|
||
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?
Updated•11 years ago
|
Blocks: 911307
status-firefox29:
--- → unaffected
status-firefox30:
--- → unaffected
status-firefox31:
--- → affected
status-firefox32:
--- → affected
tracking-firefox31:
--- → ?
tracking-firefox32:
--- → ?
Keywords: regression
Updated•11 years ago
|
Whiteboard: p=0 [qa?]
Updated•11 years ago
|
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Whiteboard: p=0 [qa?] → p=3 s=it-32c-31a-30b.3 [qa-]
Comment 4•11 years ago
|
||
Attachment #8433259 -
Flags: review?(adw)
Assignee | ||
Comment 5•11 years ago
|
||
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.
Attachment #8434668 -
Flags: review?(ttaubert)
Assignee | ||
Updated•11 years ago
|
Attachment #8433259 -
Flags: review?(adw)
Updated•10 years ago
|
Whiteboard: p=3 s=it-32c-31a-30b.3 [qa-] → p=3 s=33.1 [qa-]
Updated•10 years ago
|
Attachment #8433259 -
Attachment is obsolete: true
Comment 6•10 years ago
|
||
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.
Attachment #8434668 -
Flags: review?(ttaubert) → review+
Comment 8•10 years ago
|
||
Marco, can you please re-assign this bug to Drew? He did all the work here. Thanks!
Flags: needinfo?(mmucci)
Assignee | ||
Comment 10•10 years ago
|
||
Component: General → New Tab Page
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
Comment 12•10 years ago
|
||
Drew, could you request the uplift to aurora and beta?
status-firefox33:
--- → fixed
Flags: needinfo?(adw)
Assignee | ||
Comment 13•10 years ago
|
||
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
Attachment #8440794 -
Flags: approval-mozilla-beta?
Attachment #8440794 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(adw)
Comment 14•10 years ago
|
||
Moderate risk, OK. I will accept the uplift in aurora first and take it for beta 4 then.
Updated•10 years ago
|
Attachment #8440794 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 15•10 years ago
|
||
Comment 16•10 years ago
|
||
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+
Assignee | ||
Comment 17•10 years ago
|
||
Thanks, Sylvestre. https://hg.mozilla.org/releases/mozilla-beta/rev/34e4ea875ee2
Updated•10 years ago
|
Whiteboard: p=3 s=33.1 [qa-] p=3 s=33.1 [qa-] → [tiles] 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.
Description
•