1348613 - restoreHistory() uses sync IPC

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement)

Description

[(no longer active)]

<http://searchfox.org/mozilla-central/rev/c9f5cc6b4593d461e87a6221dc0286b3859fd515/browser/components/sessionstore/content/content-sessionStore.js#198>

https://perfht.ml/2nzLovJ

Comment 1

[Mike Conley (:mconley) (:⚙️)]

I suspect some of the work that is happening in bug 1256472 will actually help with this. Will follow up once that lands.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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

Comment 3

[Mike Conley (:mconley) (:⚙️)]

A number of test failures when just switching to async messages: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7889efa671db92cf6099484880408524eec34c7&selectedJob=84871522

Comment 4

[(no longer active)]

There is some evidence in bug 1312373 suggesting that bug 1256472 didn't really help all that much.  You should look at this profile to see what I mean! https://perfht.ml/2n0zHtQ

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Sorry, I should have been far more clear when I made that comment (comment 1). I was already aware that there were going to be some test failures with this patch, but because I was dealing with the same tests failing in bug 1256472, I thought my work there would help.

Unfortunately, as evidenced by comment 3, it did not. So some work is still required here to figure out whether or not the test failures are due to brittle tests or due to stuff being actually broken with the straight-up swap over to async messages.

Comment 6

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

I added the "SessionStore:restoreHistoryComplete" sync message. It actually only needs to be sync in non-e10s. So you could check the process type for that.

I don't remember why SessionStore:restoreTabContentStarted is sync.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(In reply to Bill McCloskey (:billm) from comment #6)
> I added the "SessionStore:restoreHistoryComplete" sync message. It actually
> only needs to be sync in non-e10s. So you could check the process type for
> that.
> 

Yeah, I'll draft up a patch to do that. I'll get my head wrapped around _why_ restoreHistoryComplete needed to be sync and file a follow-up bug to see if we can remove it from the non-e10s case (though I guess that's probably less important).

> I don't remember why SessionStore:restoreTabContentStarted is sync.

I'll follow the hg breadcrumbs and try to figure that one out too.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

The SessionStore:restoreTabContentStarted sync message was added in bug 1100223, but I can't find a justification for it.

I believe the SessionStore:restoreHistoryComplete sync message is getting around the fact that with non-e10s, the nsIWebProgress listeners are called synchronously via a mechanism that is not the in-process message queue. If we use an async message, the SessionStore:restoryHistoryComplete message occurs "too late" for us to clear the userTypedValue in the URL bar in the failing tests.

In e10s mode, we're using the message queue to send nsIWebProgress events to the parent, so despite using async messages, we get the ordering guarantees of the message queue, so the parent processes the async messages in the right order.

I think the shortest path to success is probably what billm suggested, which is to only use a sync message if we're in non-e10s mode.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1348613 - Make SessionStore:restoreTabContentStarted message always async.

Review commit: https://reviewboard.mozilla.org/r/123934/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123934/

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1348613 - Only use sync message for SessionStore:restoreHistoryComplete for non-remote tabs.

Review commit: https://reviewboard.mozilla.org/r/123936/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123936/

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8851664 [details]
Bug 1348613 - Make SessionStore:restoreTabContentStarted message always async.

https://reviewboard.mozilla.org/r/123934/#review126454

Can't really argue a one-character change, now can I? ;-P

::: commit-message-cc537:1
(Diff revision 1)
> +Bug 1348613 - Make SessionStore:restoreTabContentStarted message always aync. r?mikedeboer

nit: async

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8851665 [details]
Bug 1348613 - Only use sync message for SessionStore:restoreHistoryComplete for non-remote tabs.

https://reviewboard.mozilla.org/r/123936/#review126456

faster=better. Thanks!

::: browser/components/sessionstore/content/content-sessionStore.js:205
(Diff revision 1)
> +      // For remote tabs, because all nsIWebProgress notifications are sent
> +      // asynchronously using messages, we get the same-order guarantees of the
> +      // message manager, and can use an async message.
> -    sendSyncMessage("SessionStore:restoreHistoryComplete", {epoch, isRemotenessUpdate});
> +      sendSyncMessage("SessionStore:restoreHistoryComplete", {epoch, isRemotenessUpdate});
> +    } else {
> +      sendAsyncMessage("SessionStore:restoreHistoryComplete", {epoch, isRemotenessUpdate});

My gut tells me that our test suite is going to either 1) trigger more intermittents due to things being more - properly, mind you! - async, or 2) solve intermittents because mixing sync with async is never a good idea.

I don't know which one of the two, but I hope 2) ;-)

Good to keep an eye out for it, anyway.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8851664 [details]
Bug 1348613 - Make SessionStore:restoreTabContentStarted message always async.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123934/diff/1-2/

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8851665 [details]
Bug 1348613 - Only use sync message for SessionStore:restoreHistoryComplete for non-remote tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123936/diff/1-2/

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Thanks for the super fast reviews! :D

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d0966159a9c
Make SessionStore:restoreTabContentStarted message always async. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/ce66e1f2a032
Only use sync message for SessionStore:restoreHistoryComplete for non-remote tabs. r=mikedeboer

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/4d0966159a9c
https://hg.mozilla.org/mozilla-central/rev/ce66e1f2a032