1434768 - Basic structure for new session history APIs

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: Part 1: Add Child/ParentSHistory classes which wrap nsISHistory

The basic idea behind this patch is to introduce two new classes, ChildSHistory
and ParentSHistory. These classes are currently very thin wrappers over
nsISHistory and don't do anything fancy. The first step is to replace the
sessionHistory objects with these new objects, using the `legacySHistory`
functions to call the existing methods when needed.

After this patch, I will work on adding the necessary APIs directly to
Child/ParentSHistory to remove callers of legacySHistory, which should allow me
to eventually move the session history state into the parent process.

MozReview-Commit-ID: 9Zl7zIuBlLm

Comment 2

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

Attachment: Part 2: Replace nsDocShell::mSessionHistory with ChildSHistory

MozReview-Commit-ID: 8rFPZjlU13Y

Comment 3

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

Attachment: Part 3: Rewrite JS consumers of .sessionHistory

MozReview-Commit-ID: 5ysTaEKQynA

Comment 4

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

Comment on attachment 8947281 [details] [diff] [review]
Part 1: Add Child/ParentSHistory classes which wrap nsISHistory

Clearing review because I think I want ChildSHistory and ParentSHistory to use webidl interfaces rather than xpidl interfaces, as it'll make them nicer to use.

Comment 5

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

Attachment: Part 1: Add Child/ParentSHistory classes which wrap nsISHistory

MozReview-Commit-ID: BjcmllgBid5

Comment 6

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

Attachment: Part 2: Replace nsDocShell::mSessionHistory with ChildSHistory

MozReview-Commit-ID: Dy5I0Ikb3Tv

Comment 7

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

Attachment: Part 3: Rewrite JS consumers of .sessionHistory

MozReview-Commit-ID: 8zpKmFaNUMo

Comment 8

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

Attachment: Part 4: Create a ParentSHistory in nsFrameLoader

MozReview-Commit-ID: 52XMILRa3gE

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

What is the reason for all the work here? I mean, what is the context here?

Comment 10

[Boris Zbarsky [:bzbarsky]]

The general context is wanting a better process-switching story for toplevel navigations.  That means storing session history in the parent process so it can smoothly transition across such process switches.

This is one of the steps on the path to site isolation.

Comment 11

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #10)
> The general context is wanting a better process-switching story for toplevel
> navigations.  That means storing session history in the parent process so it
> can smoothly transition across such process switches.
> 
> This is one of the steps on the path to site isolation.

The above is correct :-)

We also need to start moving session history into the parent process for when we want to start working with out of process iframes, because multiple processes will need to have a coherent shared understanding of the current session.

The hope is also that this will have privacy benefits as we will be moving your session history out of the content process, so if a page gets access to content process memory it can't see where else you've been in your current session as easily.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8947657 [details] [diff] [review]
Part 1: Add Child/ParentSHistory classes which wrap nsISHistory

So first, a general comment on the ChildSHistory API.

This API needs to support the operations in the WebIDL History interface.  In terms of actual history entry manipulation, those are:

  length, back, forward, go, pushState, replaceState

The one that's worrying me a bit is "go".  It's currently built on top of nsISHistory by getting the index, adding the given offset, and then going to the new index.  But in the new world, the global index is shared across all frames.  So we can have to concurrent go(-1) calls which will read the same (locally cached) index, then both add -1, then both go to the same new index.  This will get sent to the parent, presumably, and not do the right thing, it seems to me.  The right thing is to go back two entries in the history.

So I think the GotoIndex API, even if we need it right now, is _not_ an API we want to support long-term on the child side.  We could name it legacyGotoIndex for the moment to deal with that.  We probably _do_ want a Go(aOffset) API, though.

I suppose we might need a GotoIndex on the child side for the _parent_ to call, maybe.  As long as that's implemented directly via poking docshells and not via talking to the parent.

>+++ b/docshell/shistory/ChildSHistory.cpp
>+  : mHistory(static_cast<nsSHistory*>(aHistory))

We better have a patch under this one which marks nsISHistory builtinclass.  It's not right now!

I did check and we don't seem to have any JS impls of it, including in comm-central, so that's good.

>+ChildSHistory::CanGoForward()
>+  bool res = false;
>+  mHistory->GetCanGoForward(&res);
>+  return res;

Might be better to expicitly return false if GetCanGoForward returns error...

>+  mHistory->GetCanGoBack(&res);

Similar here.

>+nsresult
>+ChildSHistory::GoForward()

Why does this return nsresult?  If this is meant to throw exceptions to JS, it should be marked [Throws] in the IDL and have an ErrorResult instead....

>+nsresult
>+ChildSHistory::GoBack()

Same here.

>+nsresult
>+ChildSHistory::GotoIndex(int32_t aIndex)

And here.

>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(ChildSHistory)

Do we not need to CC mHistory?  Seems like we should.

>+  JS::RootedObject result(cx);
>+  ChildSHistoryBinding::Wrap(cx, this, this, aGivenProto, &result);
>+  return result;

We generate a helper to do that for you.  So this can just be:

  return ChildSHistoryBinding::Wrap(cx, this, aGivenProto);

>+++ b/docshell/shistory/ChildSHistory.h
>+// The ChildSHistory class is an attempt to rework the way that we implement
...

It would be good if this comment described the actual purpose of this class, not what it's doing right now.  So something like:

  ChildSHistory represents a view of the session history from a child process.
  It exposes getters for some cached overall history state, and mutators that
  are implemented by communicating with the actual history storage in ParentSHistory.

etc.  Basically the comment that should be there when this work is done.

And maybe a separate comment about the current state of things.

The comment should probably also have this form:

  /**
   * Comment bits
   * go here
   */

And come right after the license block, before the header guard.  We have some tools that look for comments of that form in that location to describe the purpose of files.

>+//  - Don't add a method to nsIChildSHistory unless it is needed there, prefer
>+//    implementing methods directly

There is no nsIChildSHistory.

>+  nsISupports* GetParentObject() const { return nullptr; }

This is sub-optimal.  It will create the wrapper for this object in whatever global first accesses it, which will then entrain that global, especially if we ever preserve the wrapper.

Given the scoping of these objects, this should return either xpc::NativeGlobal(xpc::PrivilegedJunkScope()) or xpc::NativeGlobal(xpc::UnprivilegedJunkScope()).  And maybe we could skip the xpc::NativeGlobal bit if we added a JSObject* overload of dom::FindAssociatedGlobal.  This can be done as a followup.

The actual methods on this class need to be documented.

>+++ b/docshell/shistory/ParentSHistory.cpp
>+  if (XRE_IsContentProcess()) {

As you noted on IRC, can't happen.

>+ParentSHistory::WrapObject(JSContext* cx, JS::Handle<JSObject*> aGivenProto)
>+  JS::RootedObject result(cx);
>+  ParentSHistoryBinding::Wrap(cx, this, this, aGivenProto, &result);
>+  return result;

  return ParentSHistoryBinding::Wrap(cx, this, aGivenProto);

>+++ b/docshell/shistory/ParentSHistory.h
>+// The ParentSHistory class is an attempt to rework the way that we implement

Again, this should describe the thing this class actually does, plus the formatting nits from above.

>+// Currently ParentSHistory is implemented by (sometimes asynchronously)
>+// forwarding the actual work to the legacy APIs

No, it's not.  ;)

>+  nsISupports* GetParentObject() const { return nullptr; }

Same comments as for child SH.

>+  explicit ParentSHistory(nsDocShell* aDocShell);
>+  explicit ParentSHistory(TabParent* aTabParent);

How do these differ?  Document.

And document the other methods.

>+  // NOTE: Only one of these will be non-null, depending on whether we are in-process or out-of-process.
>+  RefPtr<TabParent> mTabParent;
>+  RefPtr<nsDocShell> mDocShell;

Which one in which case?  ;)

>+++ b/docshell/shistory/nsSHistory.h
>+class nsDocShell;

Seems to not be used yet...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8947658 [details] [diff] [review]
Part 2: Replace nsDocShell::mSessionHistory with ChildSHistory

The concept of GetRootSessionHistory will presumably need to be reworked going forward somehow.  Nothing you should do about it in this patch, but maybe file a bug depending on this one?

> nsDocShell::RemoveFromSessionHistory()

This is duplicating work that GetRootSessionHistory does, both before and after your changes.  Followup to clean up, perhaps.

>+nsDocShell::InitSessionHistory()
>+  nsCOMPtr<nsISHistory> history = do_CreateInstance(NS_SHISTORY_CONTRACTID);

You know you want an actual nsSHistory here.  Just use |new nsSHistory| to create one, please.

>+nsDocShell::GetSessionHistoryXPCOM(nsISupports** aSessionHistory)
>+  RefPtr<ChildSHistory> shistory = mSessionHistory.get();

Why do you need the .get() here?

>@@ -11902,36 +11858,29 @@ nsDocShell::AddState(JS::Handle<JS::Value> aData, const nsAString& aTitle,

>+  RefPtr<ChildSHistory> rootSH = GetRootSessionHistory();
>   if (!aReplace) {

You lost this bit:

    NS_ENSURE_TRUE(rootSH, NS_ERROR_UNEXPECTED);

which we do need here, I expect.

>+++ b/docshell/base/nsIWebNavigation.idl
>+interface nsIChildSHistory;

No such thing.

>+++ b/docshell/shistory/ParentSHistory.cpp
>+  return mDocShell->GetSessionHistory();

mDocShell can be null, right?

r=me with the nits fixed but the JS changes in the next changeset presumably should be folded into this one when landing, since the JS bits won't work right on top of this changeset as-is.

Comment 14

[Alan Jeffrey [:ajeffrey]]

Had some IRC chat at https://mozilla.logbot.info/content/20180202#c14236876

TL;DR: The approach being suggested here is quite similar to Servo's, with the child content processes playing the
role of Servo's script threads, and the parent frameloader playing the role of Servo's constellation.

One difference is that Servo doesn't cache much state in the content process, and uses sync ipc messaging
for each query. The usual trade-off for caching applies here: efficiency vs the headaches of cache
coherence. This is made worse by security considerations that mean we have to be careful about which state
is cached where.

Implementing a 100% spec-compatible caching implementation is tricky, because the spec is written in
a sequential style, and there are probably non-sequentially-consistent observable behaviours. For example, if two x-origin iframes both run the code `if (history.length == 5) { history.back(); }` then in any sequential execution only one of them will go back. There are probably other weird concurrency issues, but how seriously to take them is a value judgement.

One gotcha from Servo was that closing an iframe is required to be synchronous, which means that when
a close iframe message is sent to the constellation, it sends back a list of closed descendent browsing context ids.
This impacts history, in that changes to history.length etc. also need to be synchronous. In Servo, we achieve
that by making every such history query use sync ipc messages to the constellation, but if it's being cached
then I'm not sure how to achieve that.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8947659 [details] [diff] [review]
Part 3: Rewrite JS consumers of .sessionHistory

>+++ b/browser/base/content/web-panels.js

>     getPanelBrowser().webNavigation
>                      .sessionHistory
>+                     .legacySHistory
>                      .QueryInterface(nsIWebNavigation)
>                      .reload(nsIWebNavigation.LOAD_FLAGS_NONE);

Can you not cut out both the .legacySHistory and .QI instead?

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

Oh, and we should document in the webidl what the reload flags are...

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8947660 [details] [diff] [review]
Part 4: Create a ParentSHistory in nsFrameLoader

r=me

Comment 18

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

Attachment: Part 1: Add Child/ParentSHistory classes which wrap nsISHistory

Hopefully the newly cleaned up interfaces seem nicer to work with :-)

MozReview-Commit-ID: BjcmllgBid5

Comment 19

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

Attachment: Part 2: Replace nsDocShell::mSessionHistory with ChildSHistory

MozReview-Commit-ID: Dy5I0Ikb3Tv

Comment 20

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

Attachment: Part 3: Rewrite JS consumers of .sessionHistory

MozReview-Commit-ID: 8zpKmFaNUMo

Comment 21

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

Attachment: Part 5: Add an API to ParentSHistory for getting a list of SH Entries

MozReview-Commit-ID: 9689NMK5eXd

Comment 22

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

Attachment: Part 6: Remove the unnecessary shistory xpcom constructors

MozReview-Commit-ID: 3WPuGEjibtq

Comment 23

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

Attachment: Part 7: Add EvictLocalContentViewers to ChildSHistory

MozReview-Commit-ID: EKFtGFd8Itv

Comment 24

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

Attachment: Part 4: Create a ParentSHistory in nsFrameLoader

Hmm, I somehow managed to vanish part 4... It's back now.

MozReview-Commit-ID: 52XMILRa3gE

Comment 25

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

Attachment: Part 5: Add an API to ParentSHistory for getting a list of SH Entries

MozReview-Commit-ID: 9689NMK5eXd

Comment 26

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

Attachment: Part 6: Remove the unnecessary shistory xpcom constructors

MozReview-Commit-ID: 3WPuGEjibtq

Comment 27

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

Attachment: Part 7: Add EvictLocalContentViewers to ChildSHistory

MozReview-Commit-ID: EKFtGFd8Itv

Comment 28

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

Comment on attachment 8950373 [details] [diff] [review]
Part 5: Add an API to ParentSHistory for getting a list of SH Entries

I've reconsidered and I really don't like this API, so I'm clearing the review on this patch.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8950365 [details] [diff] [review]
Part 1: Add Child/ParentSHistory classes which wrap nsISHistory

I'm really sorry for the lag here...

>+++ b/docshell/shistory/ChildSHistory.cpp
>+ChildSHistory::CanGo(int32_t aOffset)
>+{
>+  CheckedInt<int32_t> index = Index();
>+  index += aOffset;
>+  if (!index.isValid()) {
>+    return false;
>+  }
>+  return index.value() < Count();

Shouldn't this also return false if index < 0?
>+// NOTE: mHistory is not cycle collectable, so we don't need to vsit it.

"visit"

I would really prefer we just visit it.  It'll be a no-op for now, that's OK.  And it's safer than not visiting it if we do make it CCed.

r=me

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8950374 [details] [diff] [review]
Part 6: Remove the unnecessary shistory xpcom constructors

r=me

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8950375 [details] [diff] [review]
Part 7: Add EvictLocalContentViewers to ChildSHistory

r=me, but we'll need to think a bit about what should happen with non-local viewers in nsFrameLoader::SwapWithOtherLoader...  Maybe a followup bug with a comment pointing to it?

Comment 32

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #29)
> I'm really sorry for the lag here...

Np!

> Shouldn't this also return false if index < 0?

Yup. For some reason I wrote the CheckedInt and assumed it handled negative numbers (even though I made it signed >.<)

> I would really prefer we just visit it.  It'll be a no-op for now, that's
> OK.  And it's safer than not visiting it if we do make it CCed.

OK

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #31)
> Comment on attachment 8950375 [details] [diff] [review]
> Part 7: Add EvictLocalContentViewers to ChildSHistory
> 
> r=me, but we'll need to think a bit about what should happen with non-local
> viewers in nsFrameLoader::SwapWithOtherLoader...  Maybe a followup bug with
> a comment pointing to it?

I mention this in the OOP iframe bug & in another history bug. It's definitely on the radar but we don't have the infrastructure to do anything about it yet.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47c477fefd3b
Part 1: Add Child/ParentSHistory classes which wrap nsISHistory, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1dc65552300
Part 2: Replace nsDocShell::mSessionHistory with ChildSHistory, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bc418e5727e
Part 3: Rewrite JS consumers of .sessionHistory, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/0171a483cd7b
Part 4: Create a ParentSHistory in nsFrameLoader, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ae896b5ad63
Part 5: Add EvictLocalContentViewers to ChildSHistory, r=bz

Comment 34

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/47c477fefd3b
https://hg.mozilla.org/mozilla-central/rev/c1dc65552300
https://hg.mozilla.org/mozilla-central/rev/3bc418e5727e
https://hg.mozilla.org/mozilla-central/rev/0171a483cd7b
https://hg.mozilla.org/mozilla-central/rev/7ae896b5ad63