Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Loic, Assigned: ttaubert)

Tracking

Trunk
Firefox 13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 verified)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Blocks: 455553
Component: Untriaged → Tabbed Browser
OS: Windows 7 → All
Hardware: x86_64 → All

Updated

6 years ago
QA Contact: untriaged → tabbed.browser
(Assignee)

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: New Tab Page: clearing all history doesn't remove thumbnails until next restart → [New Tab Page] clear internal links cache on 'browser:purge-session-history'
Version: 12 Branch → Trunk
(Assignee)

Updated

6 years ago
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
Created attachment 598196 [details] [diff] [review]
patch v1
Attachment #598196 - Flags: review?(dietrich)
Comment on attachment 598196 [details] [diff] [review]
patch v1

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

looks good, needs a test.
Attachment #598196 - Flags: review?(dietrich) → feedback+
(Assignee)

Comment 3

6 years ago
Created attachment 602450 [details] [diff] [review]
patch v2

Test added. Contains small fixes.
Attachment #598196 - Attachment is obsolete: true
Attachment #602450 - Flags: review?(dietrich)
(Assignee)

Comment 4

6 years ago
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 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.
Attachment #602450 - Flags: review?(dietrich) → review+
(Assignee)

Comment 6

6 years ago
Created attachment 602849 [details] [diff] [review]
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.
Attachment #602450 - Attachment is obsolete: true
Attachment #602849 - Flags: review?(dietrich)
Attachment #602849 - Flags: review?(dietrich) → review+
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/integration/fx-team/rev/ad37c208588d
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
Depends on: 733329
(Assignee)

Comment 8

6 years ago
Backed out because of leaks and bug 733329:

https://hg.mozilla.org/integration/fx-team/rev/cb9f5f4cbb1e
Target Milestone: Firefox 13 → ---
(Assignee)

Updated

6 years ago
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ad37c208588d
Target Milestone: --- → Firefox 13
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
backed out: 

https://hg.mozilla.org/mozilla-central/rev/cb9f5f4cbb1e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 13 → ---
(Assignee)

Comment 11

5 years ago
Created attachment 604199 [details] [diff] [review]
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.
Attachment #602849 - Attachment is obsolete: true
Attachment #604199 - Flags: review?(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?
Attachment #604199 - Flags: review?(dietrich) → review+
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

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

Comment 15

5 years ago
https://hg.mozilla.org/mozilla-central/rev/151a006fd5d1
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
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.
status-firefox13: --- → verified
You need to log in before you can comment on or make changes to this bug.