Send SHEntry info from content processes to the chrome process

NEW
Unassigned

Status

()

P2
normal
5 months ago
3 months ago

People

(Reporter: njn, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 months ago
It's unused.
Attachment #9018945 - Flags: review?(nika)
(Reporter)

Updated

5 months ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Reporter)

Comment 2

5 months ago
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.
Attachment #9018946 - Flags: feedback?(nika)

Updated

5 months ago
Attachment #9018945 - Flags: review?(nika) → review+

Comment 3

5 months ago
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 :-)
Attachment #9018946 - Flags: feedback?(nika) → feedback+
Priority: -- → P2
(Reporter)

Updated

5 months ago
Assignee: n.nethercote → nobody
Status: ASSIGNED → NEW
(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.
You need to log in before you can comment on or make changes to this bug.