restoreHistory() uses sync IPC

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Session Restore
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: Ehsan, Assigned: mconley)

Tracking

(Blocks: 1 bug)

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
(Assignee)

Comment 1

5 months ago
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)
(Assignee)

Comment 2

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=394b63ae03e8
(Assignee)

Comment 3

5 months ago
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
(Assignee)

Comment 5

5 months ago
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.
(Assignee)

Comment 7

5 months ago
(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)
(Assignee)

Comment 8

5 months ago
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

5 months 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

5 months 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)
(Assignee)

Comment 15

5 months ago
Thanks for the super fast reviews! :D

Comment 16

5 months 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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4d0966159a9c
https://hg.mozilla.org/mozilla-central/rev/ce66e1f2a032
Status: NEW → RESOLVED
Last Resolved: 5 months 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.