Closed Bug 952998 Opened 11 years ago Closed 10 years ago

Use FrameTree to collect DOMSessionStorage data

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 29

People

(Reporter: ttaubert, Assigned: ttaubert)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file)

0002-Bug-952998-Use-FrameTree-to-collect-DOMSessionStorag.patch
23.59 KB, patch
Yoric
: review+
Details | Diff | Splinter Review 
We currently collect sessionStorage data for every frame and subframe contained in a tab's session history. This is quite problematic for sites like Google Search that store a lot of data in sessionStorage but are mostly hidden in some tab's previous history entries.

I thus propose saving sessionStorage data only for the current frameTree, i.e. the currently shown page, including its subframes that finished loading before the "load" event was fired.

At the same time, we can remove the "browser:purge-session-history" observer as purging the session history does never involve the current history entry. This could still be done by loading about:blank.
What were the results here? Any improvement?
(In reply to Dietrich Ayala (:dietrich) from comment #3)
> What were the results here? Any improvement?

This certainly results in storing less DOMSessionStorage data. Only after this lands we can see whether telemetry values for collection time [1] and file size [2] decrease, which they should just because we discard more data than before.

[1] http://telemetry.mozilla.org/#nightly/29/FX_SESSION_RESTORE_COLLECT_DATA_MS
[2] http://telemetry.mozilla.org/#nightly/29/FX_SESSION_RESTORE_FILE_SIZE_BYTES
Comment on attachment 8351273 [details] [diff] [review]
0002-Bug-952998-Use-FrameTree-to-collect-DOMSessionStorag.patch

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

::: browser/components/sessionstore/src/FrameTree.jsm
@@ +165,5 @@
> +   * Applies the given function |cb| to all frames stored in the tree. Use this
> +   * method if |map()| doesn't suit your needs and you want more control over
> +   * how data is collected.
> +   *
> +   * @param cb (function)

Nit: Could you document the args passed to |cb|?

::: browser/components/sessionstore/src/SessionStorage.jsm
@@ +19,5 @@
> +  try {
> +    let uri = Services.io.newURI(doc.documentURI, null, null);
> +    return ssm.getDocShellCodebasePrincipal(uri, docShell);
> +  } catch (e) {
> +    // This throws for invalid URLs.

Please make sure that you're catching only for invalid URLs. Catch all clauses are generally to be avoided.

@@ +69,2 @@
>      let data = {};
> +    let visited = new Set();

"visited" is a little terse. Maybe |visitedOrigins|?

::: browser/components/sessionstore/test/browser_sessionStorage.js
@@ +18,5 @@
>    // Flush to make sure chrome received all data.
>    SyncHandlers.get(browser).flush();
>  
>    let {storage} = JSON.parse(ss.getTabState(tab));
> +  is(storage["http://example.com"].test, "inner-value",

Could you randomize a little these values to ensure that we don't have false positives caused by some weird persistence?

@@ +28,5 @@
>    yield modifySessionStorage(browser, {test: "modified"});
>    SyncHandlers.get(browser).flush();
>  
>    let {storage} = JSON.parse(ss.getTabState(tab));
> +  is(storage["http://example.com"].test, "inner-value",

Why don't we also modify the inner-value?

::: browser/components/sessionstore/test/content.js
@@ +33,5 @@
>    }
>  });
>  
> +addMessageListener("ss-test:purgeDomainData", function ({data: domain}) {
> +  Services.obs.notifyObservers(null, "browser:purge-domain-data", domain);

That's in content?
Attachment #8351273 - Flags: review?(dteller) → review+
I have high hopes for this patch, by the way.
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #5)
> ::: browser/components/sessionstore/src/SessionStorage.jsm
> @@ +19,5 @@
> > +  try {
> > +    let uri = Services.io.newURI(doc.documentURI, null, null);
> > +    return ssm.getDocShellCodebasePrincipal(uri, docShell);
> > +  } catch (e) {
> > +    // This throws for invalid URLs.
> 
> Please make sure that you're catching only for invalid URLs. Catch all
> clauses are generally to be avoided.

doc.documentURI should always be valid. getDocShellCodebasePrincipal() says that it "might throw" in the current code. I removed the try-catch completely because it points to a deeper issue should that fail.

> ::: browser/components/sessionstore/test/browser_sessionStorage.js
> @@ +18,5 @@
> >    // Flush to make sure chrome received all data.
> >    SyncHandlers.get(browser).flush();
> >  
> >    let {storage} = JSON.parse(ss.getTabState(tab));
> > +  is(storage["http://example.com"].test, "inner-value",
> 
> Could you randomize a little these values to ensure that we don't have false
> positives caused by some weird persistence?

Done.

> @@ +28,5 @@
> >    yield modifySessionStorage(browser, {test: "modified"});
> >    SyncHandlers.get(browser).flush();
> >  
> >    let {storage} = JSON.parse(ss.getTabState(tab));
> > +  is(storage["http://example.com"].test, "inner-value",
> 
> Why don't we also modify the inner-value?

Done.

> ::: browser/components/sessionstore/test/content.js
> @@ +33,5 @@
> >    }
> >  });
> >  
> > +addMessageListener("ss-test:purgeDomainData", function ({data: domain}) {
> > +  Services.obs.notifyObservers(null, "browser:purge-domain-data", domain);
> 
> That's in content?

AFAIU we will be notified in the content process. If that assumption doesn't hold we can easily fix this later.
(In reply to Tim Taubert [:ttaubert] from comment #7)
> > > +    let uri = Services.io.newURI(doc.documentURI, null, null);

Looks like you want doc.documentURIObject?
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #9)
> (In reply to Tim Taubert [:ttaubert] from comment #7)
> > > > +    let uri = Services.io.newURI(doc.documentURI, null, null);
> 
> Looks like you want doc.documentURIObject?

Yeah, I forgot about that. Thanks :)

Pushed a small follow-up:

https://hg.mozilla.org/integration/fx-team/rev/1e870df73ef5
https://hg.mozilla.org/mozilla-central/rev/3f6e82f8f9f8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Keywords: perf
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: