Closed Bug 1348613 Opened 3 years ago Closed 3 years ago

restoreHistory() uses sync IPC

Categories

(Firefox :: Session Restore, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: ehsan, Assigned: mconley)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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)
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 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 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+
Thanks for the super fast reviews! :D
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
https://hg.mozilla.org/mozilla-central/rev/4d0966159a9c
https://hg.mozilla.org/mozilla-central/rev/ce66e1f2a032
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.