[e10s] Remove sync TabState.flush() call from SessionStore.duplicateTab()

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

(Blocks 1 bug)

Trunk
Firefox 41
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(e10sm8+, firefox40 affected, firefox41 fixed)

Details

Attachments

(5 attachments, 5 obsolete attachments)

12.45 KB, patch
billm
: review+
Details | Diff | Splinter Review
5.04 KB, patch
billm
: review+
Details | Diff | Splinter Review
13.57 KB, patch
billm
: review+
Details | Diff | Splinter Review
2.96 KB, patch
billm
: review+
Details | Diff | Splinter Review
7.98 KB, patch
billm
: review+
Details | Diff | Splinter Review
SessionStore.duplicateTab() is a sync API that returns the duplicated tab. The nice thing however is that it doesn't make any promises about when the tab itself starts loading or starts being restored. Most of the existing tests use promiseTabRestored() already to wait until that has finished so we shouldn't break too much here and can introduce async flushing.
Duplicate of this bug: 939764
Points: --- → 5
Flags: qe-verify-
Flags: firefox-backlog+
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
This introduces the TabStateFlusher that keeps track of async flushes. Using this we can in a follow-up convert tests to:

yield TabStateFlusher.flush(browser);

That should work in most of the places. I need to think what to do about TabState.flushAsync() that is used to simulate some race conditions in browser_broadcast.js. We should keep this functionality but also get rid of the SyncHandler. That's a follow-up though.
Attachment #8603248 - Flags: review?(wmccloskey)
This makes SessionStore.duplicateTab() use the new flusher.
Attachment #8603249 - Flags: review?(wmccloskey)
With those few fixes sessionstore tests succeed locally, e10s & non-e10s.
Attachment #8603250 - Flags: review?(wmccloskey)
Guess we also need a few tests to ensure flushing works as expected.
Test that covers duplicateTab().
Attachment #8603322 - Flags: review?(wmccloskey)
Small fix to let it use Promise.jsm.
Attachment #8603248 - Attachment is obsolete: true
Attachment #8603248 - Flags: review?(wmccloskey)
Attachment #8603336 - Flags: review?(wmccloskey)
Fixes a small issue, discovered when writing tests.
Attachment #8603249 - Attachment is obsolete: true
Attachment #8603249 - Flags: review?(wmccloskey)
Attachment #8603337 - Flags: review?(wmccloskey)
The tests weren't quite checking the right things.
Attachment #8603322 - Attachment is obsolete: true
Attachment #8603322 - Flags: review?(wmccloskey)
Attachment #8603339 - Flags: review?(wmccloskey)
Tests for only flushing itself.
Attachment #8603342 - Flags: review?(wmccloskey)
Should've run tests locally before attaching :/
Attachment #8603337 - Attachment is obsolete: true
Attachment #8603337 - Flags: review?(wmccloskey)
Attachment #8603353 - Flags: review?(wmccloskey)
Needed to fix some more tests.
Attachment #8603250 - Attachment is obsolete: true
Attachment #8603250 - Flags: review?(wmccloskey)
Attachment #8603356 - Flags: review?(wmccloskey)
(Try looks great.)
(In reply to Tim Taubert [:ttaubert] from comment #2)
> I need to think what to do about
> TabState.flushAsync() that is used to simulate some race conditions in
> browser_broadcast.js. We should keep this functionality but also get rid of
> the SyncHandler. That's a follow-up though.

Oh wow. I just remembered that flushAsync() only exists to test a few edge cases that only exist because we have sync flushes. My day just got brighter.
Comment on attachment 8603336 [details] [diff] [review]
0001-Bug-1162871-Introduce-the-TabStateFlusher-for-async-.patch, v2

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

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +659,5 @@
>     *
>     * @param options (object)
>     *        {id: 123} to override the update ID used to accumulate data to send.
>     *        {sync: true} to send data to the parent process synchronously.
> +   *        {flushID: 123} to specify that this is a flush

Can you document isFinal while you're here?
Attachment #8603336 - Flags: review?(wmccloskey) → review+
Comment on attachment 8603353 [details] [diff] [review]
0002-Bug-1162871-Use-async-flushing-for-duplicateTab.patch, v3

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

I'd like to wait and see what you think about the comment below.

::: browser/components/sessionstore/SessionStore.jsm
@@ +1836,4 @@
>  
> +      // Update state with flushed data.
> +      let options = {includePrivateData: true};
> +      TabState.copyFromCache({linkedBrowser: browser}, tabState, options);

It occurs to me that copyFromCache doesn't deal with the case where browser.__SS_data is present. Based on [1] (which is used by TabState.clone), it seems like that could be a problem. This might also be an issue for bug 1109875 too.

Even if this isn't a bug, I think it might be nice to have a single API call in TabState that returns a promise that will be resolved with the tabState. Having to use copyFromCache seems like an implementation detail that shouldn't be exposed to SessionStore.jsm.

It would also be nice to fold this new API in with the bug 1109875 stuff too. Perhaps the big branch in the SessionStore:update handler that deals with _closedTabs could instead be moved to onTabClose and written to use TabStateFlusher and a promise.

Not all of this needs to happen right away. I'm just trying to think of what the desired end-state could be.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/TabState.jsm#223
Attachment #8603353 - Flags: review?(wmccloskey)
Attachment #8603356 - Flags: review?(wmccloskey) → review+
Comment on attachment 8603339 [details] [diff] [review]
0004-Bug-1162871-Test-that-duplicateTab-does-wait-for-the.patch, v2

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

::: browser/components/sessionstore/test/head.js
@@ +34,5 @@
>  Cu.import("resource:///modules/sessionstore/TabState.jsm", tmp);
> +Cu.import("resource:///modules/sessionstore/TabStateFlusher.jsm", tmp);
> +let {
> +  Promise, Task, SessionStore, SessionSaver, SessionFile, TabState,
> +  TabStateFlusher

Might be nicer if each one is on a separate line.
Attachment #8603339 - Flags: review?(wmccloskey) → review+
Comment on attachment 8603342 [details] [diff] [review]
0005-Bug-1162871-Test-TabStateFlusher.flush-behavior-incl.patch

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

::: browser/components/sessionstore/test/browser_async_flushes.js
@@ +74,5 @@
> +  yield Promise.all([promise1, promise2]);
> +
> +  // The pending update should be lost.
> +  ({entries} = JSON.parse(ss.getTabState(tab)));
> +  is(entries.length, 1, "still only one history entry");

This test seems kinda flaky. Isn't it possible that the update message will, by chance, be sent before the content process gets the crash message (due to the one second timer expiring)?
Attachment #8603342 - Flags: review?(wmccloskey) → review+
Iteration: 40.3 - 11 May → 41.1 - May 25
(In reply to Bill McCloskey (:billm) from comment #20)
> ::: browser/components/sessionstore/test/browser_async_flushes.js
> @@ +74,5 @@
> > +  yield Promise.all([promise1, promise2]);
> > +
> > +  // The pending update should be lost.
> > +  ({entries} = JSON.parse(ss.getTabState(tab)));
> > +  is(entries.length, 1, "still only one history entry");
> 
> This test seems kinda flaky. Isn't it possible that the update message will,
> by chance, be sent before the content process gets the crash message (due to
> the one second timer expiring)?

Line 48 flushes just before clicking the link. Before the second is over we crashed the browser already so this shouldn't be a problem. We have other tests doing the same, they basically rely on the assumption that sending a few messages causing small actions shouldn't take a second - so far that's been true. It's really hard to test messaging without it.
Depends on: 1166757
(In reply to Bill McCloskey (:billm) from comment #18)
> It occurs to me that copyFromCache doesn't deal with the case where
> browser.__SS_data is present. Based on [1] (which is used by
> TabState.clone), it seems like that could be a problem. This might also be
> an issue for bug 1109875 too.

Yeah, that's true it doesn't deal with it. But TabStateCache is basically a clone of __SS_data except a few things. I think we should just get rid of that and remove any chance of overriding data here. I couldn't get it to fail with multiple situations in tests so I think it's safe, it's still very confusing. Filed bug 1166757.
(In reply to Bill McCloskey (:billm) from comment #18)
> Even if this isn't a bug, I think it might be nice to have a single API call
> in TabState that returns a promise that will be resolved with the tabState.
> Having to use copyFromCache seems like an implementation detail that
> shouldn't be exposed to SessionStore.jsm.
> 
> It would also be nice to fold this new API in with the bug 1109875 stuff
> too. Perhaps the big branch in the SessionStore:update handler that deals
> with _closedTabs could instead be moved to onTabClose and written to use
> TabStateFlusher and a promise.

Bug 1109875 does really only want cached data and I think it's fine to know about that. We can't flush at this point anymore and the <xul:tab> is gone too. Other points in the browser might want to collect data without flushing, some wants to - like duplicateTab(). I think we should combine those use cases as they turn up.
(In reply to Tim Taubert [:ttaubert] from comment #21)
> Line 48 flushes just before clicking the link. Before the second is over we
> crashed the browser already so this shouldn't be a problem. We have other
> tests doing the same, they basically rely on the assumption that sending a
> few messages causing small actions shouldn't take a second - so far that's
> been true. It's really hard to test messaging without it.

OK, I see what you mean. Seems fine.
Comment on attachment 8603353 [details] [diff] [review]
0002-Bug-1162871-Use-async-flushing-for-duplicateTab.patch, v3

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

OK, let's do this then.
Attachment #8603353 - Flags: review+
Blocks: 1167502
You need to log in before you can comment on or make changes to this bug.