restoreHistory() uses sync IPC

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Session Restore
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: Ehsan, Assigned: mconley)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

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

https://perfht.ml/2nzLovJ
I suspect some of the work that is happening in bug 1256472 will actually help with this. Will follow up once that lands.
Flags: needinfo?(mconley)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=394b63ae03e8
A number of test failures when just switching to async messages: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7889efa671db92cf6099484880408524eec34c7&selectedJob=84871522
Blocks: 1312373
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
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.
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.
(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.
Assignee: nobody → mconley
Flags: needinfo?(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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

a month ago
mozreview-review
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
Attachment #8851664 - Flags: review?(mdeboer) → review+

Comment 12

a month ago
mozreview-review
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.
Attachment #8851665 - Flags: review?(mdeboer) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Thanks for the super fast reviews! :D

Comment 16

a month ago
Pushed by mconley@mozilla.com:
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

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4d0966159a9c
https://hg.mozilla.org/mozilla-central/rev/ce66e1f2a032
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.