Closed Bug 1263696 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- affected
firefox49 --- fixed

People

(Reporter: qdot, Assigned: qdot)

References

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 5 obsolete files)

Followup to bug 1262184, make embed nodes not load if they have an object element ancestor with its own content.
Whiteboard: btpp-active
Summary: Block embed content loading when ancestor is object element with content → Block embed content loading when ancestor is object element with usable content
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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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?
Attachment #8740097 - Attachment is obsolete: true
Attachment #8744513 - Flags: feedback?(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...
Flags: needinfo?(kyle)
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?
Flags: needinfo?(kyle)
Attachment #8744513 - Flags: feedback?(bzbarsky) → feedback+
Or for that matter you could have a <div> and then a sibling <embed>.  ;)
Ok, I /think/ this is the right way to traverse the node tree, and the tests cover the basic cases.
Attachment #8745647 - Attachment is obsolete: true
Attachment #8745830 - Flags: review?(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.
Attachment #8745830 - Flags: review?(bzbarsky) → review-
Traversal cleaned up, now load all valid embeds, added more tests.
Attachment #8745830 - Attachment is obsolete: true
Attachment #8745873 - Flags: review?(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!
Attachment #8745873 - Flags: review?(bzbarsky) → review+
Addressed review comments, fixed windows.h symbol undef.
Attachment #8745873 - Attachment is obsolete: true
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.
Flags: needinfo?(bzbarsky)
> Is a self closing object (<object ... />) valid?

In HTML, no.  Or rather, it's just an unclosed <object>.  That test needs </object>.
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/134d9f44aeb8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1272651
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.