1263696 - Block embed content loading when ancestor is object element with usable content

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Followup to bug 1262184, make embed nodes not load if they have an object element ancestor with its own content.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) Block embed content loading when ancestor of object element with content

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Trying to figure out how things would work if the object tag hits the fallback case, since we've already parsed the embed element and skipped calling LoadObject. Should I just check for an embed element child when we hit the fallback case code and try to load it again there?

Comment 3

[Boris Zbarsky [:bzbarsky]]

That's probably the simplest thing.

The other option is that if an <embed> finds that it should not load due to an <object> up the tree that's not falling back, it could add itself to some list on that <object>.  Then we'd have to worry about removing it from the list if it got removed from under the <object>, though, so I'm not sure it's worth it.

In practice, I expect there to be very few pages with large subtrees under <object>s that fall back, so just walking the subtree at that point seems fine.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2): WIP - Block embed content loading when ancestor of object element with content

Ok, got a loading order issue here. Turns out that we already search descendants of a node when we have a fallback. However, I'm not quite sure how to deal with trying to load an embed with content at that point? Just calling StartObjectLoad ends up with LoadObject failing because we don't have a channel. Is there a better way to reload the embed node contents?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Just calling StartObjectLoad ends up with LoadObject failing because we don't have a channel.

Er... don't have a channel at which point?

Anyway, you want to end up calling nsObjectLoadingContent::LoadObject but possibly passing "true" for the aForceLoad arg.  Though if we never called LoadObject before, doesn't seem like that should be needed...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8744513 [details] [diff] [review]
Patch 1 (v2): WIP - Block embed content loading when ancestor of object element with content

You can't stop at the fist non-param thing, unfortunately.  You might have <embed> nested somewhere under you (e.g. under a <div>).  Please add a test for that.

>+    // If we have an ancestor that is an object with a source, it'll have an
>+    // associated loading type.

Except it's called the displayed type, not the loading type, right?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Or for that matter you could have a <div> and then a sibling <embed>.  ;)

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3): Block embed content loading when ancestor of object element with content

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v4): Block embed content loading when ancestor of object element with content

Ok, I /think/ this is the right way to traverse the node tree, and the tests cover the basic cases.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8745830 [details] [diff] [review]
Patch 1 (v4):  Block embed content loading when ancestor of object element with content

Why are we only looking for the first <embed>?  We should load _all_ our descendant <embed>s, no?  Worth adding a testcase for this too.

You just want to walk over the subtree rooted at the <object> using nsINode::GetNextNode.

>+      object->LoadObject(false, true);

This is wrong.  We want to call something that will check BlockEmbedContentLoading() again before calling LoadObject, because the embed might still be blocked.  For example if it's nested in another, not-falling-back, <object>.  Again, need a test.

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v5): Block embed content loading when ancestor of object element with content

Traversal cleaned up, now load all valid embeds, added more tests.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8745873 [details] [diff] [review]
Patch 1 (v5):  Block embed content loading when ancestor of object element with content

>       if (!child->IsHTMLElement(nsGkAtoms::param) &&
>-          nsStyleUtil::IsSignificantChild(child, true, false)) {
>+            nsStyleUtil::IsSignificantChild(child, true, false)) {

Please add

  aType != eFallbackAlternate

to the start of that if, so we don't keep testing kids of IsSignificantChild (which is not cheap) once we've found a significant child.

This is subtly different from the behavior we used to have before in the case of <param> that has kids, but I don't think we care about that, honestly.  That can only happen with manual DOM manipulation or XML.  But worth pointing out that we really only care about immediate kids for changing aType; it's just that non-immediate kids are probably going to be found only after aType == eFallbackAlternate anyway.

>+        if (child->IsHTMLElement(nsGkAtoms::embed)) {
>+          HTMLSharedObjectElement* object = HTMLSharedObjectElement::FromContent(child);

Since you added NS_IMPL_FROMCONTENT_HTML_WITH_TAG to HTMLSharedObjectElement, HTMLSharedObjectElement::FromContent will already do the tag check for you, and return null if the tag is not embed.

That's actually a bit weird.  I'd rather we not add NS_IMPL_FROMCONTENT_HTML_WITH_TAG and just static_cast here directly.

>+            object->StartObjectLoad(false, true);

Should be (true, true) for the arguments, I would think.  Why did you decide to have it not notify?

r=me with those addressed.  Thank you for doing this!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/541618e4babd

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v6): Block embed content loading when ancestor of object element with content; r=bz

Addressed review comments, fixed windows.h symbol undef.

Comment 15

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac1770eeff8a

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Is a self closing object (<object ... />) valid? This bug is failing mochitests because the embed at https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/general/performance_timeline_main_test.html#97 thinks the object tag above it is actually a parent even though it closes itself. Adding </object> fixes it.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Is a self closing object (<object ... />) valid?

In HTML, no.  Or rather, it's just an unclosed <object>.  That test needs </object>.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/134d9f44aeb8

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/134d9f44aeb8