1596776 - nsDocShellLoadState(DocShellLoadStateInit&) doesn't set mSHEntry

Status: RESOLVED FIXED

(Core :: DOM: Navigation, enhancement, P2)

Description

[Alphan Chen [:alchen]]

https://searchfox.org/mozilla-central/rev/cac340f10816707e91c46db6d285f80259420f07/dom/ipc/DOMTypes.ipdlh#256
We don't support mSHEntry now.
It also needs to be serialized in nsDocShellLoadState::Serialize.

Comment 1

[Alphan Chen [:alchen]]

With bug 1596077, we use "BrowsingContext->LoadURI()" in nsSHistory.cpp to prevent docShell is null case.

In this case, we use "Canonical()->GetCurrentWindowGlobal()->SendLoadURIInChild(aLoadState, aSetNavigating);" in "BrowsingContext->LoadURI()"
https://searchfox.org/mozilla-central/rev/e7c61f4a68b974d5fecd216dc7407b631a24eb8f/docshell/base/BrowsingContext.cpp#869

Because we don't set mSHEntry into DocShellLoadStateInit, we lost it when arriving WindowGlobalChild::RecvLoadURIInChild().
Then we cannot complete sessionHistory.reloadCurrentEntry() when using it from the parent process.

Comment 2

[Alphan Chen [:alchen]]

I think this is part of the implementation of sessionHistory in parent(Bug 1438272).
Bug 1539482 is also related to nsISHEntry in IPC call.

Hi Peter and Anny,
could you take a look on this?

Comment 3

[Alphan Chen [:alchen]]

(In reply to Alphan Chen [:alchen] from comment #2)

I think this is part of the implementation of sessionHistory in parent(Bug 1438272).
Bug 1539482 is also related to nsISHEntry in IPC call.

The problem is not caused by Bug 1539482.
This call flow comes from sessionHistory.reloadCurrentEntry().
There is no this kind of call flow before enabling session history in the parent process.
At that time, we always call sessionHistory.reloadCurrentEntry() from the child process.

Comment 4

[Anny Gakhokidze [:annyG]]

Some additional notes as well on other things that need to be fixed - when I created CreateLoadInfo() here https://searchfox.org/mozilla-central/rev/131338e5017bc0283d86fb73844407b9a2155c98/docshell/shistory/nsISHEntry.idl#422, we decided to set the shentry on the nsDocShellLoadState in SHEntryChild::CreateLoadInfo instead, when session history in parent in enabled, to avoid (de)serializing the entry. With session history in parent disabled, this path https://searchfox.org/mozilla-central/rev/131338e5017bc0283d86fb73844407b9a2155c98/docshell/shistory/SHEntryChild.cpp#991-993 is never invoked and thus we never set it. I should fix this as well.

Comment 5

[Alphan Chen [:alchen]]

STR:

1. apply the patch(https://phabricator.services.mozilla.com/D46281)
2. Run browser/components/sessionstore/test/browser_formdata.js
3. Check logs and result. Now we have two failures for this test.

[Current log]
0:06.34 GECKO(9588) 870@BrowsingContext::LoadURI aLoadResult.SHEntry() exists
...
0:06.34 GECKO(9588) ----DEBUG--------------- nsDocShell::LoadURI:795[0x7fbbf8050800]: InternalLoad() without aLoadState->SHEntry()
...
0:06.63 FAIL form data not restored - Got value-0.6022411674343265, expected

[Expected log]
0:06.34 GECKO(9588) 870@BrowsingContext::LoadURI aLoadResult.SHEntry() exists
...
0:06.34 GECKO(9588) ----DEBUG--------------- nsDocShell::LoadURI:763[0x7fbbf8050800]:loading from session history
...
0:06.63 PASS form data not restored

Comment 6

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1596776 - Serialize SHEntry when sending DocShellLoadStateInit over IPC, r=peterv

Comment 7

[Alphan Chen [:alchen]]

Attachment: Nov-25-mochitest_1.log

By applying this patch, sometimes I found the following logs when doing the test.
GECKO(30539) IPDL protocol error: could not lookup id for PSHEntry
GECKO(30539) IPDL protocol error: Error deserializing MaybeNewPSHEntry
It happened randomly when testing the whole folder. (./mach test browser/components/sessionstore/test/)
But it won't happen if I only run a single test.

[From Nov-25-mochitest_1.log]
0:14.94 GECKO(5654) aLoadResult.SHEntry() exists
0:14.94 GECKO(5654) 870@BrowsingContext::LoadURI aLoadResult.SHEntry() exists
0:14.94 GECKO(5654) BrowsingContext::LoadURI mDocShell not exists, XRE_IsParentProcess()=1
0:14.94 GECKO(5654) ====DEBUG==== @ SessionStore.jsm receiveMessage(SessionStore:restoreTabContentStarted), data={"epoch":1,"isRemotenessUpdate":false,"reason":0}!!!
0:14.94 GECKO(5654) IPDL protocol error: could not lookup id for PSHEntry
0:14.94 GECKO(5654) IPDL protocol error: Error deserializing MaybeNewPSHEntry

2:43.25 GECKO(5654) IPDL protocol error: could not lookup id for PSHEntry
2:43.25 GECKO(5654) IPDL protocol error: Error deserializing MaybeNewPSHEntry

4:13.31 GECKO(5654) IPDL protocol error: could not lookup id for PSHEntry
4:13.31 GECKO(5654) IPDL protocol error: Error deserializing MaybeNewPSHEntry
4:13.31 GECKO(5654) ----DEBUG--------------- nsDocShell::LoadURI:795[0x7f1d80255800]: InternalLoad() without aLoadState->SHEntry()

4:59.11 GECKO(5654) IPDL protocol error: could not lookup id for PSHEntry
4:59.11 GECKO(5654) IPDL protocol error: Error deserializing MaybeNewPSHEntry

6:28.52 GECKO(5654) IPDL protocol error: could not lookup id for PSHEntry
6:28.52 GECKO(5654) IPDL protocol error: Error deserializing MaybeNewPSHEntry

7:17.87 GECKO(5654) IPDL protocol error: could not lookup id for PSHEntry
7:17.87 GECKO(5654) IPDL protocol error: Error deserializing MaybeNewPSHEntry
7:18.09 GECKO(5654) ----DEBUG--------------- nsDocShell::LoadURI:795[0x7f6c8d6ac800]: InternalLoad() without aLoadState->SHEntry()

Comment 8

[Alphan Chen [:alchen]]

Here is the same error found on try.
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=278074212&repo=try&lineNumber=6440
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=278074162&repo=try&lineNumber=15505

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fde732c4e940
Serialize SHEntry when sending DocShellLoadStateInit over IPC, r=peterv

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/fde732c4e940