[New Tab Page] AllPages.unregister() can possibly remove wrong pages

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 633489 [details] [diff] [review]
trivial patch

When given a page that isn't in the list of registered pages, NewTabUtils.allPages.unregister() will still remove a page because it doesn't check if indexOf() > -1.
Attachment #633489 - Flags: review?(mak77)
Comment on attachment 633489 [details] [diff] [review]
trivial patch

How'd you find this? Presumably in practice unregister never gets called for pages that haven't been registered?
Attachment #633489 - Flags: review?(mak77) → review+
(Assignee)

Comment 2

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #1)
> How'd you find this? Presumably in practice unregister never gets called for
> pages that haven't been registered?

Right, while working on bug 753448 I noticed that the preloaded about:newtab instances were out of sync because they weren't tracked anymore. This happens because of how we use registerCleanupFunction() here to make sure the dummy page gets removed when timing out:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/newtab/head.js#312
(Assignee)

Comment 3

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/78bae12834f6
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 16
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/mozilla-central/rev/78bae12834f6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.