Bug 88148

git bisect fingers r118098.

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?

(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?

(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/>

> > 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.

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.

Created attachment 145540 [details] Patch

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

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).

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.

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.

Created attachment 145719 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

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

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

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

Created attachment 145736 [details] Patch

Created attachment 145913 [details] Patch

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?

Created attachment 146473 [details] Patch

> 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.

(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?

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.)

(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.

Created attachment 146791 [details] Patch

Created attachment 146798 [details] Patch

(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.

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?

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.

/me touches nose. I deferred to you first!

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

Comment on attachment 146798 [details] Patch \o/

Created attachment 146997 [details] Patch for landing

(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.

Created attachment 147013 [details] Patch for landing

Comment on attachment 147013 [details] Patch for landing Clearing flags on attachment: 147013 Committed r120051: <http://trac.webkit.org/changeset/120051>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 88852

Created attachment 147211 [details] Patch for landing

Comment on attachment 147211 [details] Patch for landing Clearing flags on attachment: 147211 Committed r120168: <http://trac.webkit.org/changeset/120168>

All reviewed patches have been landed. Closing bug.