REGRESSION(r118098): <content> element does not render distributed children when cloned from another document

• Component: DOM
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

There is something really wrong happening here. This is the functionality used by Web Components Polyfill and this stopped working very recently (like, this week).

[Dominic Cooney]

git bisect fingers r118098.

[Dominic Cooney]

The cause of this seems simple enough: The function which wraps a HTMLContentElement tries to access the "context" (ie frame) via the element being wrapped. Independent documents don’t have frames.

A temporary workaround for the Web Components polyfill would be to create a document in a same-origin iframe.

What is the intended design of ContextEnabledFeatures? Should it access the current JavaScript context, like the SOP does?

[Dimitri Glazkov (Google)]

(In reply to comment #2)
> The cause of this seems simple enough: The function which wraps a HTMLContentElement tries to access the "context" (ie frame) via the element being wrapped. Independent documents don’t have frames.
> 

I see. The decision is made at the creation time and is permanent. So even if the object is moved from one  document to another, its nature stays the same. Hmm... This seems deficient.

BTW, this is not just about wrappers. The code that breaks us is the one generated for HTMLElementFactory.cpp, which is used by the parser.

> A temporary workaround for the Web Components polyfill would be to create a document in a same-origin iframe.

Yep, that's what I'll do in the meantime.

> 
> What is the intended design of ContextEnabledFeatures? Should it access the current JavaScript context, like the SOP does?

[Dominic Cooney]

(In reply to comment #3)
> I see. The decision is made at the creation time and is permanent. So even if the object is moved from one  document to another, its nature stays the same. Hmm... This seems deficient.

Ah… maybe a less intrusive workaround than using an iframe, since you are cloning the node anyway and then adopting it, is to use importNode. This will create a new node in the new document.

adoptNode can return a new node, but it doesn’t. So it could be special-cased for elements of "context enabled features." It feels a bit sad to layer all this complexity in here – although <content> specifically is a presentation feature, so relying on having a window is arguably OK, you can certainly observe whether you got HTMLContentElement or HTMLUnknownElement from script.

One question I still need clarified – what is the intended "context"? Per window? Origin? (Morita-san?)

> BTW, this is not just about wrappers. The code that breaks us is the one generated for HTMLElementFactory.cpp, which is used by the parser.

Good point.

> > A temporary workaround for the Web Components polyfill would be to create a document in a same-origin iframe.
> 
> Yep, that's what I'll do in the meantime.

Maybe importNode is your friend. <http://jsfiddle.net/QWt5j/>

[Hajime Morrita]

> 
> One question I still need clarified – what is the intended "context"? Per window? Origin? (Morita-san?)
> 
it should correspond domain. I choose (main) frame as its proxy. 
A document created by createHTMLDocument() is frame-less and breaks this approximation.
We (well, I) need to figure out some way to reach the main frame.

[Hajime Morrita]

Here is my current plan:

- Extract ContextEnabledFeatureClient from FrameLoaderClient
- Add Document::m_contextEnabledFeatureClient
- Set it on the document construction.

The lifetime of the client should be handled carefully.
But basically it should work. I'll explore this direction at first.

[Hajime Morrita]

Created attachment 145540 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 145540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145540&action=review

Interesting! So the ContextEnabledFeatures now becomes a thing like SecurityOrigin. Can it just be part of SecurityOrigin?

> Source/WebCore/page/Frame.h:65
> +class ContextEnabledFeatures;

Indent mismatch.

> Source/WebCore/page/Frame.h:290
> +inline ContextEnabledFeatures* Frame::features() const
> +{
> +    return m_features.get();
> +}

Ditto.

> LayoutTests/fast/dom/shadow/elements-in-frameless-document.html:8
> +description("This test ensure that the shadow related elements are instantiated even inside frameless documents.");

ensure -> ensures

[Adam Barth]

This is related to the general problem of not being able to find the Settings object in some cases.  We kind of randomly guess at the value of Settings in that case.  That cause some trouble for us when we were bringing up the HTML5 parser because we used a Setting to determine which parser to use and hand to pick one of them when we couldn't find the Settings object.

When adding a new settings, is it going to be obvious whether to add it to the Page-specific settings or the Frame-specific settings?  These seem like the fulfill almost the same purpose.  I guess it's uncommon to need Frame-specific settings since we've gotten along thus far without them.

There's a slight risk in how you've got things set up currently.  Imagine the embedder wants to enable a feature for all documents in the HTTPS scheme and to not expose the feature to documents in the HTTP scheme, the embedder can't really do that the way you've got things set up.  When the embedder receives one of these calls and has only a Frame upon which to base its decision.  It doesn't know *which* of the documents in the Frame is asking the question.

A better design might be to use a Page-level client and then to pass a Document as a context object.  Because the client is at the Page level, it's easier for the embedder to remember that it doesn't quite know the context for the question.  (Most people remember that there are more than one Frame per Page better than they remember that there are more than one Document per Frame).

[Hajime Morrita]

Thanks Adam, I'd take that way.
Considering the requirement to access Settings from frameless document,
It might be better to introduce something like DocumentSettings or DocumentPolicy,
which asks a client to decide.

[Hajime Morrita]

My feeling is(In reply to comment #8)
> (From update of attachment 145540 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145540&action=review
> 
> Interesting! So the ContextEnabledFeatures now becomes a thing like SecurityOrigin. Can it just be part of SecurityOrigin?
Yeah, I feel same. It looks SecurityOrigin maps some concrete concept in the standard.
So we can introduce something which includes SecurityOrigin.

[Hajime Morrita]

Created attachment 145719 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Build Bot]

Comment on attachment 145719 [details]
Patch

Attachment 145719 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12896494

[Early Warning System Bot]

Comment on attachment 145719 [details]
Patch

Attachment 145719 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12890653

[Build Bot]

Comment on attachment 145719 [details]
Patch

Attachment 145719 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12899463

[Hajime Morrita]

Created attachment 145736 [details]
Patch

[Hajime Morrita]

Created attachment 145913 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 145913 [details]
Patch

I like this, though I am a bit sad about the static/non-static duplication of methods in ContextFeatureSwitch. Can we just get rid of that :)?

Possibly bike-shedding idea -- can't we make features an enum? Then the switch has only one-method surface.

Adam, WDYT?

[Hajime Morrita]

Created attachment 146473 [details]
Patch

[Hajime Morrita]

> Possibly bike-shedding idea -- can't we make features an enum? Then the switch has only one-method surface.
Tried an enum version in the updated patch. Not so bad.

[Dimitri Glazkov (Google)]

(In reply to comment #21)
> > Possibly bike-shedding idea -- can't we make features an enum? Then the switch has only one-method surface.
> Tried an enum version in the updated patch. Not so bad.

Enum makes things a lot more flexible -- easier to add/remove features. Can we propagate this enum to the static functions too? This way, there wouldn't be any need to add methods per feature.

Also -- naming thoughts (of course!) Could ContextFeatureSwitch be just ContextFeatures?

[Adam Barth]

Comment on attachment 146473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146473&action=review

This is a big improvement over the previous version, but I think I've spotted one bug that you'll probably want to fix before landing.

> Source/WebCore/dom/ContextFeatureSwitch.h:40
> +class ContextFeatureSwitch : public Supplement<Page> {

So, I think there's a subtle bug here.  ContextFeatureSwitch has the same lifetime as Page, but SecurityContext holds a raw pointer to a ContextFeatureSwitch.  Document is a subclass of SecurityContext and can outlive the page.  When it does that, the Document will have a garbage pointer to a ContextFeatureSwitch.  It will then use-after-free a bunch of memory and call a virtual method of ContextFeatureSwitchClient, which is a security vulnerability.

I wonder if a better approach is to make ContextFeatureSwitch RefCounted so that SecurityContext can hold a reference to it.  You'll then probably want to make it a FrameDestructionObserver so that it can observe when the Frame goes away and stop calling through to the client at that point.  (Once the Frame goes away, there's no reason to believe anything related to the Frame, like the Page or the ContextFeatureSwitchClient still exists.)

[Hajime Morrita]

(In reply to comment #22)
> (In reply to comment #21)
> > > Possibly bike-shedding idea -- can't we make features an enum? Then the switch has only one-method surface.
> > Tried an enum version in the updated patch. Not so bad.
> 
> Enum makes things a lot more flexible -- easier to add/remove features. Can we propagate this enum to the static functions too? This way, there wouldn't be any need to add methods per feature.
> 
I'd like to keep current style since This makes caller sides less concise.

> Also -- naming thoughts (of course!) Could ContextFeatureSwitch be just ContextFeatures?
Can be. ContextFeaturesClient sounds a bit for me. That's why I chose this name.
But it doesn't look so bad now. Will try.

[Hajime Morrita]

Created attachment 146791 [details]
Patch

[Hajime Morrita]

Created attachment 146798 [details]
Patch

[Hajime Morrita]

(In reply to comment #23)
> (From update of attachment 146473 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146473&action=review
> 
> This is a big improvement over the previous version, but I think I've spotted one bug that you'll probably want to fix before landing.
> 
> > Source/WebCore/dom/ContextFeatureSwitch.h:40
> > +class ContextFeatureSwitch : public Supplement<Page> {
> 
> So, I think there's a subtle bug here.  ContextFeatureSwitch has the same lifetime as Page, but SecurityContext holds a raw pointer to a ContextFeatureSwitch.  Document is a subclass of SecurityContext and can outlive the page.  When it does that, the Document will have a garbage pointer to a ContextFeatureSwitch.  It will then use-after-free a bunch of memory and call a virtual method of ContextFeatureSwitchClient, which is a security vulnerability.

Right. Actually I encountered similar situation before.

IMHO, some of Supplement classes want to be ref-counted, and they also want to know
associated Page's destruction timing to clear the will-be-invalidated references.
In the updated patch, I introduced RefCountedSupplement for this.
I agree that FrameDestructionObserver works. 
But this will reflect Page's lifecycle directly, which is we're interested in this case.

I think this can applicable to some supplement-based Web API implementation too.
Some of them will be able to kill redundant controller classes with this.

[Adam Barth]

Comment on attachment 146798 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146798&action=review

> Source/WebCore/dom/ContextFeatures.cpp:53
> +    document->setFeatureSwitch(provided);

What is a Switch?  Do you mean a command-line switch?  If so, that seems like an odd thing to leak into these names.

> Source/WebCore/dom/ContextFeatures.h:49
> +    static void provideTo(Document*, Page*);

It's unclear whether we're providing ContextFeatures to the Document or to the Page...  I'm not sure what to suggest to improve these names.

> Source/WebCore/dom/ContextFeatures.h:83
> +void provideContextFeaturesTo(Page*, ContextFeaturesClient*);

It's strange that this is a free function but ContextFeatures::provideTo is a member function.

> Source/WebCore/dom/SecurityContext.h:98
> +    RefPtr<ContextFeatures> m_featureSwitch;

It's a bit strange to store this here given that it only really works with Documents.  Why not store it in Document?

> Source/WebCore/platform/RefCountedSupplement.h:47
> +        virtual bool isRefCountedWrapper() const OVERRIDE { return true; }

Should this be debug-only given that it's just used for an ASSERT?

[Adam Barth]

LGTM.  My comments above are just silly naming things.  I'm happy to do the final review, or I'm also happy to defer to dglazkov or whoever else is interested.

[Dimitri Glazkov (Google)]

/me touches nose. I deferred to you first!

[Hajime Morrita]

Thanks guys!
I'll update the patch to address the comment. Don't hesitate r+ then ;-)

[Dimitri Glazkov (Google)]

Comment on attachment 146798 [details]
Patch

\o/

[Hajime Morrita]

Created attachment 146997 [details]
Patch for landing

[Hajime Morrita]

(In reply to comment #28)
> (From update of attachment 146798 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146798&action=review
> 
> > Source/WebCore/dom/ContextFeatures.cpp:53
> > +    document->setFeatureSwitch(provided);
> 
> What is a Switch?  Do you mean a command-line switch?  If so, that seems like an odd thing to leak into these names.

Good catch. This should be renamed.

> 
> > Source/WebCore/dom/ContextFeatures.h:49
> > +    static void provideTo(Document*, Page*);
> 
> It's unclear whether we're providing ContextFeatures to the Document or to the Page...  I'm not sure what to suggest to improve these names.
> 
Moved to this as a function and give more descriptive name.

> > Source/WebCore/dom/ContextFeatures.h:83
> > +void provideContextFeaturesTo(Page*, ContextFeaturesClient*);
> 
> It's strange that this is a free function but ContextFeatures::provideTo is a member function.
> 
> > Source/WebCore/dom/SecurityContext.h:98
> > +    RefPtr<ContextFeatures> m_featureSwitch;
> 
> It's a bit strange to store this here given that it only really works with Documents.  Why not store it in Document?
> 
> > Source/WebCore/platform/RefCountedSupplement.h:47
> > +        virtual bool isRefCountedWrapper() const OVERRIDE { return true; }
> 
> Should this be debug-only given that it's just used for an ASSERT?
True. Will fix.

[Hajime Morrita]

Created attachment 147013 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 147013 [details]
Patch for landing

Clearing flags on attachment: 147013

Committed r120051: <http://trac.webkit.org/changeset/120051>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 88852

[Hajime Morrita]

Created attachment 147211 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 147211 [details]
Patch for landing

Clearing flags on attachment: 147211

Committed r120168: <http://trac.webkit.org/changeset/120168>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.