Closed Bug 1400501 Opened 3 years ago Closed 3 years ago

Store a list of ancestor principals in document objects

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

53 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

There are various things like bug 1305237 and probably bug 1085214 that will want to know the list of "ancestor principals" for a document.  The way I would define that is that this list is empty for a toplevel document.  For a non-toplevel document X that is nested through document Y (in the https://html.spec.whatwg.org/multipage/browsers.html#browsing-context-nested-through sense), the list for Y is the list for X, with the principal of X prepended.

If we actually had an accessor for the "nested through" concept in the spec, we could probably use that.  GetParentDocument() doesn't work for this purpose, because we null that out when the child's docshell is navigated.  We could build something on top of getting the docshell and then getting the window and then getting the frameElement and then the ownerDocument.  But this is also moderately fragile, especially the bit about "getting the docshell", if we're talking about a navigated-away-from document.  I'd rather not rely on fragile things for security state.

Hence the proposal to just snapshot this information at document creation.  This has the benefit of playing nicer with things like out-of-process iframes too: instead of querying things sync cross-process, we just have to ship over the one immutable list once.
Right now every document in a docshell makes a copy of the list.  In practice,
this list is usually pretty short (limited by depth of iframe nesting), so this
is probably not a problem.  We could add a bit of complexity and have a
refcounted struct that contains the list... I wish we had something as simple
as Rust's Arc that we could use here.

MozReview-Commit-ID: 8jGIlkhp1DU
Attachment #8908959 - Flags: review?(michael)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8908959 [details] [diff] [review]
Store a list of ancestor principals on docshell and document

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

It wouldn't be hard to add something like rust's Arc/Rc if we thought it would have usage - probably as a wrapper around RefPtr:

template<typename T>
using Arc = RefPtr<ArcBox<T>>;

template<typename T>
class ArcBox : public T {
  //  ... constructors etc...
  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ArcBox)
};

// .. Probably a MakeArc free function to construct one?

::: docshell/base/nsDocShell.h
@@ +1130,5 @@
>    uint32_t mTouchEventsOverride;
>  
> +  // Our list of ancestor principals.
> +  nsTArray<nsCOMPtr<nsIPrincipal>> mAncestorPrincipals;
> +  

nit: whitespace

::: dom/base/nsIDocument.h
@@ +456,5 @@
> +   * ancestor list for the document's docshell the last time SetContainer() was called
> +   * with a non-null argument. See the documentation for the corresponding getter in
> +   * docshell for how this list is determined.
> +   */
> +  const nsTArray<nsCOMPtr<nsIPrincipal>>& AncestorPrincipals() const

I don't see a way that this can be mutated in the DocShell right now, so is there a reason we don't just fetch this value from the DocShell when we need it rather than performing a copy into nsIDocument?

I'm assuming there's a reason (as you did mention that you were considering using refcounting here), but I don't see it immediately.
Attachment #8908959 - Flags: review?(michael) → review+
(In reply to Michael Layzell [:mystor] from comment #2)
> I don't see a way that this can be mutated in the DocShell right now, so is
> there a reason we don't just fetch this value from the DocShell when we need
> it rather than performing a copy into nsIDocument?
> 
> I'm assuming there's a reason (as you did mention that you were considering
> using refcounting here), but I don't see it immediately.

Nevermind - I should read comment 0 ^.^
Priority: -- → P3
> nit: whitespace

Fixed.

> Nevermind - I should read comment 0 ^.^

No, I should have better code comments.  Added those:

   * We store a copy of the list, because we may lose the ability to reach our docshell
   * before people stop asking us for this information.

Filed bug 1401008 on Arc/Rc.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe05bde7b58f
Store a list of ancestor principals on docshell and document.  r=mystor
https://hg.mozilla.org/mozilla-central/rev/fe05bde7b58f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.