722273 - [New Tab Page] clear internal links cache on 'browser:purge-session-history'

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Loic]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120129 Firefox/12.0a1
Build ID: 20120129031114

Steps to reproduce:

I you clear all the history of your profile, New Tab Page removes thumbnails until next restart, but not immediately (press F5 to refresh New Tab Page and observe).

Another side effect: if you clear browsing history + cache, New Tab Page removes immediately website thumbnail but keep a blank thumbnail (gray background) with its title.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 598196 [details] [diff] [review]
patch v1

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

looks good, needs a test.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Test added. Contains small fixes.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 602450 [details] [diff] [review]
patch v2

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

Explaining the small fixes contained in the patch:

::: browser/base/content/test/newtab/head.js
@@ +133,5 @@
>  
>        // Continue when the link cache has been populated.
> +      NewTabUtils.links.populateCache(function () {
> +        executeSoon(TestRunner.next);
> +      });

We need to ensure that the test continues *after* every other callback has been called.

::: browser/modules/NewTabUtils.jsm
@@ -121,5 @@
>        // want any data from private browsing to show up.
>        PinnedLinks.resetCache();
>        BlockedLinks.resetCache();
> -
> -      Pages.update();

That's legacy code referring to a non-existant object.

@@ +559,5 @@
>    /**
>     * Resets the links cache.
>     */
>    resetCache: function Links_resetCache() {
> +    this._links = null;

The cache isn't properly re-filled when setting it to '[]'.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 602450 [details] [diff] [review]
patch v2

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

::: browser/modules/NewTabUtils.jsm
@@ +236,5 @@
>        Services.prefs.setBoolPref(PREF_NEWTAB_ENABLED, !!aEnabled);
>    },
>  
>    /**
> +   * Returns the number of register New Tab Pages (i.e. the number of open

nit: registered

@@ +525,5 @@
>        }.bind(this));
> +
> +      // Register an observer to listen for browser history sanitization.
> +      if (!this._observing) {
> +        this._observing = true;

hm. i don't know what's more resource efficient - keeping a flag around for the lifetime of the app or making a method that memoizes itself into a no-op after first run.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

(In reply to Dietrich Ayala (:dietrich) from comment #5)
> > +      // Register an observer to listen for browser history sanitization.
> > +      if (!this._observing) {
> > +        this._observing = true;
> 
> hm. i don't know what's more resource efficient - keeping a flag around for
> the lifetime of the app or making a method that memoizes itself into a no-op
> after first run.

I'd at least expect the JS engine to optimize and remove empty functions and all calls to them. Also, I like this more than keeping the flag around.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Backed out because of leaks and bug 733329:

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

Comment 9

[Rob Campbell [:rc] (:robcee)]

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

Comment 10

[Rob Campbell [:rc] (:robcee)]

backed out: 

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

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

>+++ b/browser/base/content/test/newtab/head.js
>-      cells = cw.gGrid.cells;
>-
>       // Continue when the link cache has been populated.
>-      NewTabUtils.links.populateCache(TestRunner.next);
>+      NewTabUtils.links.populateCache(function () {
>+        cells = cw.gGrid.cells;
>+        executeSoon(TestRunner.next);
>+      });

This fixes bug 733329. We shouldn't access the grid and its cells before the page has been initialized.

>   resetCache: function Links_resetCache() {
>     this._links = [];
>-  }

I needed to change this back because this is the cause of the leak. I didn't understand why this causes a leak but this fix isn't really necessary. We don't really use resetCache() in the wild, only for testing.

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 604199 [details] [diff] [review]
patch v4

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

::: browser/base/content/test/newtab/head.js
@@ +132,3 @@
>        // Continue when the link cache has been populated.
> +      NewTabUtils.links.populateCache(function () {
> +        cells = cw.gGrid.cells;

i don't really like the way cells is used globally, but that's not really part of this patch. can you file a bug to update it to at least be gCells or something?

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dietrich Ayala (:dietrich) from comment #12)
> i don't really like the way cells is used globally, but that's not really
> part of this patch. can you file a bug to update it to at least be gCells or
> something?

Yeah, right. Filed bug 734280.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/151a006fd5d1

Comment 16

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20120417 Firefox/13.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:13.0) Gecko/20120416 Firefox/13.0a2
Mozilla/5.0 (Windows NT 6.1; rv:12.0) Gecko/20120416 Firefox/13.0a2

Verified with latest Aurora on Windows 7, Mac OS 10.7 and Ubuntu 11.10. All links/thumbnails are deleted when history is cleared.