Comment on attachment 8959482 [details] [diff] [review] 0001-Bug-1445659-Create-AbstractBrowsingContexts-in-paren.patch Finally got something together. This creates an AbstractBrowsingContext in both TabParent and TabChild, like we discussed. Only TabChild populates the docshell of the ABC, and attaching + detaching hangs off of the end of nsDocShell::InternalLoad, because that's where we converge with different kinds of loads. The 0002-patch exposes a function getAbstractBrowsingContext on Window that I've used for testing, and the zip attached contains such a test. If you unzip that and run two servers locally on different ports, then you can see ABC's being built by calling 'postMessage('get', '*') in the debug console.

(In reply to Andreas Farre [:farre] from comment #4) > Comment on attachment 8959482 [details] [diff] [review] > 0001-Bug-1445659-Create-AbstractBrowsingContexts-in-paren.patch > > Finally got something together. And I should say that it only stores the bare minimum on the ABC's.

There are some ipc errors, due to the silly broadcasting in TabParent. Still very much work in progress.

Moved ABCs to docshell, where they should've been all along. Also fixed detaching, but I'm not really happy, since we have the map of all ABCs in a TabChild we can never just deref a node and assume that the entire subtree is detached.

This removes AbstractBrowsingContextDictionary in favour of having a StaticAutoPtr<nsDataHashTable<uint64_t, AbstractBrowsingContext>> of all available contexts, and a StaticAutoPtr<LinkedList<RefPtr<AbstractBrowsingContext>> of all root contexts and static ::Get + ::GetOrCreate. Attach + Detach of ABCs sort of works, but breaks for session history and some other cases. It is in no way clear when to either attach or detach a child context to a parent or a root to the list of roots, especially since it is required that the PBrowser is present.

Move Attach/Detach from PBrowser to PContent. Make child abc's refcounted.

Try is looking good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b006956495819dc4f9322e3a88889f56266605f&group_state=expanded&selectedJob=181126953

So I've been thinking a bit more about the AbstractBrowsingContext name, and despite the 'ABC' being really cute, I'm starting to think we should just call it a BrowsingContext. The original reason why we wanted not to use the name 'BrowsingContext' was because it seemed more appropriate as a name for nsDocShell. However, I think ABCs match the spec definition of a BrowsingContext, as it persists across browser navigations. The nsDocShell acts more as a "Document Loader" which handles loading a sequence of related documents within an individual process. Using the BrowsingContext name will likely require less explanation to people familiar with the standard, and is also a shorter name. How do you feel about that change :farre & :bz?

In the new world where docshell is not persistent across navigations, makes sense to me.

Wasn't the idea that AbstractBrowserContext would then have concrete, per process BrowsingContexts (== DocShells). From a process point of view, DocShell is BrowsingContext. And things like "document does not have a browsing context" in the spec refers to something closer to DocShell. And per spec BrowsingContext is the thing which loads stuff. "A browsing context is an environment in which Document objects are presented to the user." ABC which doesn't have docshell attached to it in the current process doesn't have any document objects presented to the user. But whatever works the best. I have my opinion, but it isn't strong.

git mv AbstractBrowsingContext.* -> BrowsingContext.* I'm in total agreement.

Comment on attachment 8983525 [details] [diff] [review] 0001-Bug-1445659-Make-it-possible-to-store-RefPtr-T-in-Au.patch Review of attachment 8983525 [details] [diff] [review]: ----------------------------------------------------------------- Good catch. I have no problem with the code, but please add some tests, too. :) ::: mfbt/LinkedList.h @@ +98,5 @@ > // to another, no enter or exit calls happen since the elements still belong > // to a list. > static void enterList(LinkedListElement<T>* elt) {} > static void exitList(LinkedListElement<T>* elt) {} > + static void cleanElement(LinkedListElement<T>* elt) { delete elt; } Please provide some documentation for this new method, since the previous documentation doesn't cover this method.

Boris, me and Peter have been discussing this patch while I've been implementing it, so I was wondering if you would be ok with him reviewing it? Otherwise I'd be perfectly happy for you or Olli to do it, whatever makes it easier. It has recently started to look landable, and the new BC class actually gets some use by moving name from nsDocShell to BrowsingContext.

Thanks for the catch, I had that documentation uncommitted locally. I added two _very_ simple tests. I didn't reset the r? flag since I'm a bit hesitant about those two tests being enough (and the fact that cppunittests refuse to run for me locally). Will return to it on thursday after tomorrows holiday.

It's fine with me if Peter reviews this.

Test cases added to the AutoCleanLinkedList patch. Moved to gtest and found and fixed a bug! In the previous version delete was called on LinkedListElement<T>*, which happens to have a non-virtual destructor. The solution is to call delete on LinkedListElement<T>->asT(), similarly to how the other adapter methods do.

Comment on attachment 8984076 [details] [diff] [review] 0001-Bug-1445659-Make-it-possible-to-store-RefPtr-T-in-Au.patch Review of attachment 8984076 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for writing the tests! ::: mfbt/tests/gtest/TestLinkedList.cpp @@ +23,5 @@ > + { > + EXPECT_TRUE(!*mResult); > + } > + > + virtual ~PtrClass() { Does this really need to be virtual? Or is this to work around some compiler complaint? I think you could just make `PtrClass` `final`, and then the destructor doesn't need to be virtual. Oh, bother, you inherit below. OK, never mind! @@ +65,5 @@ > +{ > +public: > + int mCount; > + void AddRef() { mCount++; } > + void Release() { mCount--; } It is weird to see a Release() method that doesn't destroy the class, but OK! @@ +71,5 @@ > + CountedClass() > + : mCount(0) > + { > + } > + virtual ~CountedClass() { EXPECT_TRUE(mCount == 0); } Same reasoning applies here, with respect to declaring the class `final` and the destructor non-virtual.

(In reply to Nathan Froyd [:froydnj] from comment #24) > Comment on attachment 8984076 [details] [diff] [review] > 0001-Bug-1445659-Make-it-possible-to-store-RefPtr-T-in-Au.patch > > Review of attachment 8984076 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thank you for writing the tests! > > ::: mfbt/tests/gtest/TestLinkedList.cpp > @@ +23,5 @@ > > + { > > + EXPECT_TRUE(!*mResult); > > + } > > + > > + virtual ~PtrClass() { > > Does this really need to be virtual? Or is this to work around some > compiler complaint? I think you could just make `PtrClass` `final`, and > then the destructor doesn't need to be virtual. > > Oh, bother, you inherit below. OK, never mind! > > @@ +65,5 @@ > > +{ > > +public: > > + int mCount; > > + void AddRef() { mCount++; } > > + void Release() { mCount--; } > > It is weird to see a Release() method that doesn't destroy the class, but OK! > > @@ +71,5 @@ > > + CountedClass() > > + : mCount(0) > > + { > > + } > > + virtual ~CountedClass() { EXPECT_TRUE(mCount == 0); } > > Same reasoning applies here, with respect to declaring the class `final` and > the destructor non-virtual. Right, this could be final. I'll make it so. The reason for the other being virtual is exactly to test that the right destructor gets called in AutoCleanLinkedList (the bug I found was actually due to that). The fact that the refcounted doesn't destroy itself is because I want to be able to inspect counts. I borrowed heavily from the cppunittests in the folder above.

r+ carried over, fixed testcase to use final.

Comment on attachment 8983526 [details] [diff] [review] 0002-Bug-1445659-Create-basic-Browsing-Context-in-Content.patch Review of attachment 8983526 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/BrowsingContext.cpp @@ +30,5 @@ > + } > + > + if (!sBrowsingContexts) { > + sBrowsingContexts = > + new nsDataHashtable<nsUint64HashKey, BrowsingContext*>(); Shouldn't this be cleared on shutdown too? Seems like we're leaking the hashtable otherwise. @@ +115,5 @@ > +void > +BrowsingContext::SetName(const nsAString& aName) > +{ > + mName = aName; > +} Might make sense to inline these simple functions. ::: docshell/base/BrowsingContext.h @@ +17,5 @@ > +class nsIDocShell; > + > +namespace mozilla { > + > +class BrowsingContext This could use a comment describing what the class represents and how it's used. Probably also explaining some of the syncing that happens across processes. @@ +33,5 @@ > + BrowsingContext(uint64_t aBrowsingContextId, const nsAString& aName); > + > + void Attach(BrowsingContext* aParent); > + void Detach(); > + Please document what this actually does. ::: docshell/base/nsDocShell.cpp @@ +3715,5 @@ > nsresult rv = RemoveChildLoader(childAsDocLoader); > NS_ENSURE_SUCCESS(rv, rv); > > + nsCOMPtr<nsIDocShell> childAsDocShell(do_QueryInterface(aChild)); > + if (!childAsDocShell) { This is backwards! (Looks like we never detach here in practice?) ::: dom/ipc/ContentChild.cpp @@ +3769,5 @@ > } > } > > +mozilla::ipc::IPCResult > +ContentChild::RecvAttachBrowsingContext( Do we really need this on ContentParent *and* ContentChild? The patch seems to only send from child to parent? ::: dom/ipc/ContentParent.cpp @@ +5727,5 @@ > +ContentParent::RecvAttachBrowsingContext( > + const BrowsingContextId& aParentId, > + const BrowsingContextId& aChildId, > + const nsString& aName) > +{ Do we need to do some checking on the IDs here? I wonder what would happen if a compromised child process ends up spoofing aParentId and/or aChildId.

Addressed review comments, and perhaps most importantly took a stab at some verification of process/context ids. Also moved BrowsingContext from mozilla to mozilla::dom as per Nika's request. Also added logging, to be able to see what's happening. The changes grew a bit out of scope so I'll attach an interdiff.

Rebased and fixed some namespace errors.

... and updated the interdiff.

Comment on attachment 8986822 [details] [diff] [review] 0002-Bug-1445659-Create-basic-Browsing-Context-in-Content.patch Review of attachment 8986822 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/BrowsingContext.h @@ +85,5 @@ > + ~BrowsingContext(); > + > + const uint64_t mBrowsingContextId; > + > + // Indicates which process that owns the docshell. Only valid in the s/that// ::: docshell/base/nsDocShell.cpp @@ +477,5 @@ > > rv = nsDocLoader::AddDocLoaderAsChildOfRoot(this); > NS_ENSURE_SUCCESS(rv, rv); > > + mBrowsingContext = new mozilla::dom::BrowsingContext(this); This file has |using namespace mozilla::dom;| so no need for mozilla::dom::, here and further down. @@ +5353,5 @@ > mSessionHistory->EvictLocalContentViewers(); > mSessionHistory = nullptr; > } > > + mBrowsingContext->Detach(); So I guess this will Detach twice for "child" BrowsingContexts? (Destroy also calls RemoveChild) ::: dom/ipc/ContentParent.cpp @@ +5757,5 @@ > + // we want? Maybe we want to crash the child currently calling > + // SendAttachBrowsingContext and/or the child that originally > + // called SendAttachBrowsingContext or possibly all children that > + // has a BrowsingContext connected to the child that currently > + // called SendAttachBrowsingContext? Can you file a bug on this? I'm worried we'll ship without making a real decision here. ::: dom/ipc/PContent.ipdl @@ +1172,5 @@ > + * BrowsingContext effectively does nothing. Note that it is not > + * an error to call DetachBrowsingContext on a BrowsingContext > + * belonging to an already detached subtree. > + */ > + async DetachBrowsingContext(BrowsingContextId aContextId); Maybe move these to just under AddPerformanceMetrics, that way there's only 1 |parent:| block.

Comment on attachment 8986822 [details] [diff] [review] 0002-Bug-1445659-Create-basic-Browsing-Context-in-Content.patch Review of attachment 8986822 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/BrowsingContext.cpp @@ +80,5 @@ > +{ > + if (isInList()) { > + MOZ_LOG(GetLog(), > + LogLevel::Debug, > + ("%s: Connecting already existing 0x%08lx to 0x%08lx\n", All these |0x%08lx| fail to compile for me on OS X, I replaced them locally with |0x%08" PRIx64 "|. ::: docshell/base/nsIDocShell.idl @@ +17,5 @@ > #include "nsIURI.h" > class nsPresContext; > class nsIPresShell; > namespace mozilla { > +class BrowsingContext; This now needs to be in the dom namespace block below.

Fixed spelling, namespace, format string, added bug for illegal ops handling (it's Bug 1471598), and moved declarations in PContent. Carrying over r+

Some test code that adds getAbstractBrowsingContext and getAllAbstractBrowsingContexts to Window. They both return objects describing the BC, useful to JSON.stringify. I usually call them from the console, which makes it possible to inspect the child process (given that you have a document loaded), but loading for exacmple about:config and calling either of the will give the BC's of the parent.

Have BrowsingContext.h be exported to mozilla/dom/, after discussion with peterv. Carrying over r+

And the final fixup for today (I promise): added a null check in ~BrowsingContext: BrowsingContext::~BrowsingContext() { MOZ_DIAGNOSTIC_ASSERT(!isInList()); - sBrowsingContexts->Remove(mBrowsingContextId); + + if (sBrowsingContexts) { + sBrowsingContexts->Remove(mBrowsingContextId); + } } Needed if a shutdown cycle collection happens after sBrowsingContexts is cleared. Carrying over r+, hope that's ok Peter.

Comment on attachment 8988189 [details] [diff] [review] 0003-Bug-1445659-Make-BrowsingContext-interact-with-bfcac.patch Review of attachment 8988189 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/BrowsingContext.cpp @@ +164,5 @@ > + false /* aMoveToBFCache */); > +} > + > +void > +BrowsingContext::Cache() I think this should be named CacheChildren or something @@ +192,5 @@ > + true /* aMoveToBFCache */); > +} > + > +bool > +BrowsingContext::IsCached() This needs to be named differently (RemoveFromCache?). It doesn't just return whether it's cached, it removes it. ::: dom/ipc/PContent.ipdl @@ +1165,5 @@ > * an error to call DetachBrowsingContext on a BrowsingContext > * belonging to an already detached subtree. > */ > + async DetachBrowsingContext(BrowsingContextId aContextId, > + bool aMoveToBFCache); Need to document aMoveToBFCache.

Fixed review issues + a crasher due to accessing sCachedBrowsingContexts in BrowsingContext::Detach() after shutdown. Carrying over r+

Added cleanup of BrowsingContexts when child processes go away. I'm note sure if I'm happy with the solution, with a lot of root contexts iterating over that list might take too much time? It would be fairly easy to instead idle-dispatch a runnable that does the work incrementally. Also, since cleanup acts on root contexts it doesn't handle the case where non-root contexts, that should be cleaned, appear in several child processes. For that I filed Bug 1472108.

Patch number 4 in this patch series iterates over _all_ BrowsingContexts in the ContentParent when a ContentChild goes away, and this might be sub-optimal. This patch initiates an incremental cleanup sequence using idle callbacks. If we decide to go with this instead it needs a bit more tweaking to determine the best timeout, etc. I'm not saying we should, but I thought I'd attach it anyway.

Try is looking greenish: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ea3c42607f4d959b6f6437e40d5efd423811dcf&group_state=expanded&selectedJob=185577345 There are some intermittents that insist on not going away after retries.

Comment on attachment 8988690 [details] [diff] [review] 0004-Bug-1445659-Remove-dangling-BrowsingContexts-left-fr.patch Review of attachment 8988690 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/BrowsingContext.cpp @@ +67,5 @@ > +// cleaned. [Bug 1472108] > +/* static */ void > +BrowsingContext::CleanupContexts(uint64_t aProcessId) > +{ > + if (sRootBrowsingContexts) { I think in the future we will want to keep these BrowsingContexts around even after the browser has crashed for use in sessionrestore, but I think this is good for now :-)

Fixed rebase conflicts, carrying over r+

Try looks like this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c482cbb181b73fd4fbfcc47db1fa7214471aa265&group_state=expanded There are some permafailing intermittents that I haven't diagnosed. Not sure what to make of it.

So I ran a try run for the permafailing tests of the parent commit of the patch series, and it looks like those problems are there too[1]. So I guess that this is green enough on try. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f09b7baa7f4cd75f78c9b97e4f40cd898e99043&group_state=expanded

Pushed by apavel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9c5f1bec2005 Make it possible to store RefPtr<T> in AutoCleanLinkedList. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/c70a61e3f34a Create basic Browsing Context in Content Parent and Child. r=peterv https://hg.mozilla.org/integration/mozilla-inbound/rev/10b4f3fa453f Make BrowsingContext interact with bfcache. r=peterv https://hg.mozilla.org/integration/mozilla-inbound/rev/5e77586d8615 Remove dangling BrowsingContexts left from closing process. r=Nika

https://hg.mozilla.org/mozilla-central/rev/9c5f1bec2005 https://hg.mozilla.org/mozilla-central/rev/c70a61e3f34a https://hg.mozilla.org/mozilla-central/rev/10b4f3fa453f https://hg.mozilla.org/mozilla-central/rev/5e77586d8615