Last Comment Bug 765211 - [New Tab Page] AllPages.unregister() can possibly remove wrong pages
: [New Tab Page] AllPages.unregister() can possibly remove wrong pages
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
:
Mentors:
Depends on:
Blocks: 753448
  Show dependency treegraph
 
Reported: 2012-06-15 05:53 PDT by Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
Modified: 2012-06-16 03:51 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
trivial patch (703 bytes, patch)
2012-06-15 05:53 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
gavin.sharp: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-06-15 05:53:19 PDT
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.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-15 10:47:57 PDT
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?
Comment 2 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-06-15 10:50:07 PDT
(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
Comment 3 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-06-15 10:57:25 PDT
https://hg.mozilla.org/integration/fx-team/rev/78bae12834f6
Comment 4 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-06-16 03:44:24 PDT
https://hg.mozilla.org/mozilla-central/rev/78bae12834f6

Note You need to log in before you can comment on or make changes to this bug.