855270 - NewTabUtils Links.getLinks can return trailing nulls

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Sam Foster [:sfoster] (he/him)]

When a pinned link is unpinned it is set to null in PinnedLinks.links. If it is the last entry this trailing null can get passed into Links.getLinks, and on to the caller as a null in the returned array if there aren't enough links (places) to fill this slot.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Attachment: pop() entry when unpining the last link, to avoid trailing nulls

This patch heads of the trailing null issue at the source, by truncating the pinned links array (via pop()) when unpinning the last link, rather than setting it to null.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 730126 [details] [diff] [review]
pop() entry when unpining the last link, to avoid trailing nulls

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

Suppose we have pinned sites like [null, null, null, null, {something}] and we unpin the last one we'd only pop on entry. I think a better way to handle this would be to search for the first non-null entry starting from the back and then use this.links.splice() to remove trailing null entries.

Comment 3

[Sam Foster [:sfoster] (he/him)]

Attachment: pop() trailing null entries when unpinning

Comment 4

[Sam Foster [:sfoster] (he/him)]

Attachment: pop() trailing null entries when unpinning

Good catch on the [null,null,{something}] case. I've reworked this so it loops from the back of the array, pop-ing null entries. As I can't think of a way to get the index of the last non-null entry without a loop, this single while loop seems to get the job done concisely.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 730148 [details] [diff] [review]
pop() trailing null entries when unpinning

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

::: toolkit/modules/NewTabUtils.jsm
@@ +411,5 @@
> +    let links = this.links;
> +    links[index] = null;
> +    // trim trailing nulls
> +    while (links.length && links[links.length-1] == null)
> +      links.pop();

How about something like:

> let i = links.length - 1;
> while (i >= 0 && links[i] == null) i--;
> links.splice(i + 1)

This way we'd only modify the array once instead of calling pop() up to N times.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: Splice off trailing null entries when unpinning

Ok, I like it. Looks good and works well for me.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 730152 [details] [diff] [review]
Splice off trailing null entries when unpinning

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

Great, thank you for discovering this, btw!

Comment 8

[Sam Foster [:sfoster] (he/him)]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e67d5f1708

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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