Implement a minimal form of Abstract Browsing Contexts

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
4 months ago

People

(Reporter: farre, Assigned: farre)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(7 attachments, 22 obsolete attachments)

1.26 KB, application/zip
Details
5.56 KB, patch
farre
: review+
Details | Diff | Splinter Review
9.70 KB, patch
Details | Diff | Splinter Review
10.38 KB, patch
farre
: review+
Details | Diff | Splinter Review
4.36 KB, patch
Nika
: review+
Details | Diff | Splinter Review
6.84 KB, patch
Details | Diff | Splinter Review
38.03 KB, patch
farre
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → afarre
Posted file test.zip
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.
Attachment #8959482 - Flags: feedback?(peterv)
(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.
Priority: -- → P2
There are some ipc errors, due to the silly broadcasting in TabParent. Still very much work in progress.
Attachment #8959482 - Attachment is obsolete: true
Attachment #8959483 - Attachment is obsolete: true
Attachment #8959482 - Flags: feedback?(peterv)
Flags: needinfo?(peterv)
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.
Attachment #8968452 - Attachment is obsolete: true
Attachment #8968453 - Attachment is obsolete: true
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.
Attachment #8973695 - Attachment is obsolete: true
Summary: Create AbstractBrowsingContexts in parent/child → Implement a minimal form of Abstract Browsing Contexts
Move Attach/Detach from PBrowser to PContent. Make child abc's refcounted.
Attachment #8974470 - Attachment is obsolete: true
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?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(afarre)
In the new world where docshell is not persistent across navigations, makes sense to me.
Flags: needinfo?(bzbarsky)
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.
Attachment #8976982 - Attachment is obsolete: true
Flags: needinfo?(afarre)
Attachment #8982831 - Attachment is obsolete: true
Attachment #8983525 - Flags: review?(nfroyd)
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.
Attachment #8983525 - Flags: review?(nfroyd) → review-
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.
Attachment #8982832 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
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.
Attachment #8983525 - Attachment is obsolete: true
Flags: needinfo?(nfroyd)
It's fine with me if Peter reviews this.
Flags: needinfo?(bzbarsky)
Attachment #8983526 - Flags: review?(peterv)
Blocks: 1467214
Blocks: 1467227
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.
Attachment #8983543 - Attachment is obsolete: true
Flags: needinfo?(nfroyd)
Attachment #8984076 - Flags: review?(nfroyd)
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.
Attachment #8984076 - Flags: review?(nfroyd) → review+
(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.
Attachment #8984076 - Attachment is obsolete: true
Attachment #8985152 - Flags: review+
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.
Attachment #8983526 - Flags: review?(peterv) → review-
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.
Attachment #8983526 - Attachment is obsolete: true
Attachment #8986453 - Flags: review?(peterv)
Attachment #8986454 - Attachment mime type: text/x-patch → text/plain
Rebased and fixed some namespace errors.
Attachment #8986453 - Attachment is obsolete: true
Attachment #8986453 - Flags: review?(peterv)
Attachment #8986822 - Flags: review?(peterv)
... and updated the interdiff.
Attachment #8986454 - Attachment is obsolete: true
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.
Attachment #8986822 - Flags: review?(peterv) → review+
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.
Depends on: 1471598
No longer depends on: 1471598
Fixed spelling, namespace, format string, added bug for illegal ops handling (it's Bug 1471598), and moved declarations in PContent.

Carrying over r+
Attachment #8986822 - Attachment is obsolete: true
Attachment #8986824 - Attachment is obsolete: true
Attachment #8988185 - Flags: review+
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+
Attachment #8988185 - Attachment is obsolete: true
Attachment #8988218 - Flags: review+
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.
Attachment #8988218 - Attachment is obsolete: true
Attachment #8988228 - Flags: review+
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.
Attachment #8988189 - Flags: review?(peterv) → review+
Attachment #8988228 - Attachment is obsolete: true
Attachment #8988465 - Flags: review+
Fixed review issues + a crasher due to accessing sCachedBrowsingContexts in BrowsingContext::Detach() after shutdown.

Carrying over r+
Attachment #8988189 - Attachment is obsolete: true
Attachment #8988467 - Flags: review+
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.
Attachment #8988690 - Flags: review?(peterv)
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.
Attachment #8988690 - Flags: review?(peterv) → review?(nika)
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 :-)
Attachment #8988690 - Flags: review?(nika) → review+
Fixed rebase conflicts, carrying over r+
Attachment #8988465 - Attachment is obsolete: true
Attachment #8995269 - Flags: review+
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
Keywords: checkin-needed
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
Keywords: checkin-needed
Flags: needinfo?(peterv)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.