1500829 - Remove nsIDocShell::shouldSaveLayoutState

Status: RESOLVED FIXED

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

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove nsIDocShell::shouldSaveLayoutState

It's unused.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (DRAFT) Send SHEntry info from content processes to the chrome process

This is a first attempt at sending SHEntry info from content processes to the 
chrome process. I've added some IPDLery for this -- a message and a struct. And
I have a printf on the receiving end for testing purposes.

It's a mess right now. I have a very poor sense of which parts of
nsISHEntry/nsSHEntry/nsSHEntryShared need to be sent to the parent, and how
they line up with what SessionHistory.jsm's collect() function gets. I've added
various annotations in comments while trying to make sense of all that. They 
might be comprehensible, they might not.

Currently I'm doing the sending only from nsSHistory::AddEntry(). We probably
need to send from various other places in nsSHistory.cpp, too.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9018946 [details] [diff] [review]
(DRAFT) Send SHEntry info from content processes to the chrome process

Review of attachment 9018946 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/shistory/nsSHEntry.h
@@ +36,5 @@
>    ~nsSHEntry();
>  
>    // We share the state in here with other SHEntries which correspond to the
>    // same document.
>    RefPtr<nsSHEntryShared> mShared;

The version of mChildShells which you send should instead

@@ +46,5 @@
> +  nsCOMPtr<nsIURI> mResultPrincipalURI;   // njn: resultPrincipalURI      .spec
> +  nsCOMPtr<nsIURI> mReferrerURI;          // njn: referrerURI                 Y
> +  uint32_t mReferrerPolicy;               // njn: referrerPolicy              Y
> +  nsString mTitle;                        // njn: title
> +  nsCOMPtr<nsIInputStream> mPostData;     // njn: postData

This could contain some big information if it's a file stream. I think there are a limited # of types of streams which can end up here, and it would be awesome to avoid piping data from the content process unnecessarily due to IPCStream.

Perhaps poke Baku to see if he knows what the best solution would be here?

For now, we can get away with not including this data in the parent process, and add it in a follow-up.

@@ +53,5 @@
> +  int32_t mScrollPositionX;               // njn: {get,set}ScrollPosition()   Y
> +  int32_t mScrollPositionY;               // njn: {get,set}ScrollPosition()   Y
> +  nsISHEntry* mParent;                    // njn: parent
> +  nsCOMArray<nsISHEntry> mChildren;       // njn: GetChildAt()                Y
> +  nsCOMPtr<nsIStructuredCloneContainer> mStateData; // njn: stateData         Y

This should be sent using StructuredCloneData :-) https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/dom/ipc/StructuredCloneData.h#85-148

::: docshell/shistory/nsSHEntryShared.h
@@ +78,1 @@
>    nsCOMArray<nsIDocShellTreeItem> mChildShells;

If you send this down, which seems like a reasonable idea, send it down as a list of BrowsingContextIDs, one for each of the docshells (as they correspond).

@@ +90,2 @@
>    nsCOMPtr<nsIDocument> mDocument;
> +  nsCOMPtr<nsILayoutHistoryState> mLayoutHistoryState;  // njn: yes

I did some work at one point to make nsILayoutHistoryState more serializable. The internal state is just a `nsDataHashtable<nsCStringHashKey, UniquePtr<PresState>> mStates;`, and PresState is a IPDL struct due to my changes.

https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/layout/base/nsLayoutHistoryState.cpp#37

@@ +90,3 @@
>    nsCOMPtr<nsIDocument> mDocument;
> +  nsCOMPtr<nsILayoutHistoryState> mLayoutHistoryState;  // njn: yes
> +  nsCOMPtr<nsISupports> mWindowState;           // njn: yes

In general nsSHEntryShared corresponds to a single document in history, which may have multiple entries (e.g. due to pushstate, hash navigations, etc.)

Because of that, some pieces of nsSHEntryShared don't need to be synced. These are generally the ones relating to the BFCache, such as mContentViewer (viewer for a cached document), mDocument (the cached document), and mWindowState (the cached window global). We can probably also skip sending down mExpirationState and mEditorData for now.

@@ +95,3 @@
>    nsExpirationState mExpirationState;
>    nsAutoPtr<nsDocShellEditorData> mEditorData;
> +  nsWeakPtr mSHistory;                          // njn: yes

This shouldn't have to be synced, as it'll be pretty implicit :-)

Comment 4

[Jan Henning [:JanH]]

(In reply to Nicholas Nethercote [:njn] (on PTO until Jan 7, 2019) from comment #2)
> Currently I'm doing the sending only from nsSHistory::AddEntry(). We probably
> need to send from various other places in nsSHistory.cpp, too.

The LayoutHistoryState is only properly updated when navigating *away* from a session history entry (see also what I've written in bug 1350567) and the same is probably true for the scroll position stored directly within the SHEntry (SHEntry->GetScrollPosition()). So whenever the currently active SHEntry changes, we also need to update at least parts of the *previously* active SHEntry again.

Comment 5

[Nicholas Nethercote [inactive]]

I stopped working on this bug a while back. But it's worth landing the first clean-up patch, so I'll repurpose the bug for that.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Bug 1500829 - Remove nsIDocShell::shouldSaveLayoutState. r=qdot

It's unused.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08d6d034d45b
Remove nsIDocShell::shouldSaveLayoutState. r=nika

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/08d6d034d45b