1167508 - Use async flushing for LoadInOtherProcess() and remove TabState.flush()

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

LoadInOtherProcess() is the only place still using TabState.flush() outside of tests.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

We can remove TabState.flush() with that already but not sync flushing quite yet as we're still using .flushWindow().

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1167508-Reset-epoch-when-receiving-XULFrameLoade.patch

I found that we need to reset the epoch when a new frameLoader is created in a <browser>. This mostly works fine currently because when we do that (remoteness switching) we at the same time restore data into the tab, which also restores the old epoch. This doesn't happen when the browser crashes though, so I think the correct way to handle this is to actually reset the epoch and not carry it forward.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1167508-Support-flushing-crashed-browsers.patch

TabStateFlusher.flush() currently doesn't support crashed browsers and does never resolve in that case. We shouldn't punish the caller for not checking whether the tab is crashed as the tab state is basically frozen and should simply be accessible.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1167508-Use-async-flushing-for-LoadInOtherProces.patch

This moves the remoteness switching code from LoadInOtherProcess() to SessionStore and makes it a regular API. Calling browser.loadURI() does now first flush asynchronously to receive all the necessary data and then start navigation. Remoteness switching happens asynchronously now.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-1167508-Remove-TabState.flush.patch

Let's get rid of TabState.flush(). We still have .flushWindow() though, the last that needs to be removed.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0005-Bug-1167508-Fix-BrowserTestUtils.browserLoaded-and-u.patch

This makes SessionStore's promiseBrowserLoaded() simply use BrowserTestUtils.browserLoaded() and fixes that latter to support being called for a browser that may switch its remoteness asynchronously while waiting for the load to happen.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0006-Bug-1167508-Fix-tests-that-expect-a-sync-remoteness-.patch

All the necessary test fixes.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc95171b4ab1

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1167508-Use-async-flushing-for-LoadInOtherProces.patch, v2

Small fix.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0006-Bug-1167508-Fix-tests-that-expect-a-sync-remoteness-.patch, v2

Small fix.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0691fd611e08

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0006-Bug-1167508-Fix-tests-that-expect-a-sync-remoteness-.patch, v3

One more test fix.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd29b64f623e

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Gah, these customizableui tests are hard, need to do some more work on those.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8609826 [details] [diff] [review]
0002-Bug-1167508-Support-flushing-crashed-browsers.patch

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

This handles the case where the browser crashed before the TabStateFlusher.flush. Even with this patch, we still won't handle a crash after a flush is sent and before the reply is received, right? Should we maybe do resolveAll in that case? We have all the data we're going to get at that time.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm a little scared looking at the rest of the patches. There's nothing wrong with the patches here. It's just the whole concept of remoteness switching is so broken. I'd like to think a little more about how to make it better. I'll get back to this tomorrow or Friday. Sorry for the delay.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Bill McCloskey (:billm) from comment #16)
> I'm a little scared looking at the rest of the patches. There's nothing
> wrong with the patches here. It's just the whole concept of remoteness
> switching is so broken. I'd like to think a little more about how to make it
> better. I'll get back to this tomorrow or Friday. Sorry for the delay.

No worries, I'm all open to better ideas here :)

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Bill McCloskey (:billm) from comment #15)
> This handles the case where the browser crashed before the
> TabStateFlusher.flush. Even with this patch, we still won't handle a crash
> after a flush is sent and before the reply is received, right? Should we
> maybe do resolveAll in that case? We have all the data we're going to get at
> that time.

SessionStore.onBrowserCrashed() calls TabStateFlusher.resolveAll() passing the browser that crashed. Added that case in bug 1162871 already :)

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8609889 [details] [diff] [review]
0003-Bug-1167508-Use-async-flushing-for-LoadInOtherProces.patch, v2

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

OK, let's just do this. I don't see any alternative.

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8610217 [details] [diff] [review]
0006-Bug-1167508-Fix-tests-that-expect-a-sync-remoteness-.patch, v3

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

::: browser/base/content/test/general/browser_e10s_chrome_process.js
@@ +24,5 @@
>      let asyncTask = Task.async(transitionTask);
> +    yield asyncTask(browser, endURL);
> +
> +    if (startProcessIsRemote != endProcessIsRemote) {
> +      yield BrowserTestUtils.waitForEvent(browser, "XULFrameLoaderCreated");

Rather than encoding this specific event into a bunch of tests, can we add a function to BTU that does it? That way, if we change how this stuff works, we won't have to change a bunch of tests.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0006-Bug-1167508-Fix-tests-that-expect-a-sync-remoteness-.patch, v4

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec955be9ccfd

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0006-Bug-1167508-Fix-tests-that-expect-a-sync-remoteness-.patch, v5

Aha, about:newtab was causing issues when it was preloaded. Shouldn't use that in tests if we're not actually testing about:newtab itself.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01a14b89656c

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0007-Bug-1167508-Cancel-pending-actions-when-after-flushi.patch

Fixing a few more failures on try that weren't there before. Turns out that some tests cause the window to be closed just before flushing completes. We need to check if the window is gone before continuing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a4353961665

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Was luck enough to hit an existing leak in bug 1172137 with my patches. Pushed to try again with the fixes, hope that's the last try push.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af54e489c668

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

(Well, I didn't hit it *in* bug 1172137 but I filed that bug to fix the leak.)

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

*sigh*

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b3959289e4a

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3a4faf863320
https://hg.mozilla.org/integration/fx-team/rev/b7ed44b4595f
https://hg.mozilla.org/integration/fx-team/rev/092037dd209a
https://hg.mozilla.org/integration/fx-team/rev/c1a16fd640ae
https://hg.mozilla.org/integration/fx-team/rev/00eb943a27b2
https://hg.mozilla.org/integration/fx-team/rev/97b5372b8950
https://hg.mozilla.org/integration/fx-team/rev/5080433ff610

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Backed out because bug 1172137 had to be backed out.

Comment 31

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/986f641d87e4

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/929721960685
https://hg.mozilla.org/integration/fx-team/rev/a7bc5772c79c
https://hg.mozilla.org/integration/fx-team/rev/0b0b75e8dd0b
https://hg.mozilla.org/integration/fx-team/rev/d3de3ab56658
https://hg.mozilla.org/integration/fx-team/rev/ad065623983e
https://hg.mozilla.org/integration/fx-team/rev/aead6e4865f9
https://hg.mozilla.org/integration/fx-team/rev/c2437b9b6626

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/929721960685
https://hg.mozilla.org/mozilla-central/rev/a7bc5772c79c
https://hg.mozilla.org/mozilla-central/rev/0b0b75e8dd0b
https://hg.mozilla.org/mozilla-central/rev/d3de3ab56658
https://hg.mozilla.org/mozilla-central/rev/ad065623983e
https://hg.mozilla.org/mozilla-central/rev/aead6e4865f9
https://hg.mozilla.org/mozilla-central/rev/c2437b9b6626