Consider returning null from contentDocument getters when the caller does not subsume the document

RESOLVED FIXED in mozilla23

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bz, Assigned: bholley)

Tracking

(Blocks: 1 bug, {dev-doc-complete, site-compat})

unspecified
mozilla23
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Apparently this is what WebKit does and Hixie would slightly like to do that to simplify the spec.  I don't think this should be particularly hard to implement.

Specifically, the relevant properties are, afaict:  

1)  .contentDocument on HTMLFrameElement, HTMLIFrameElement, HTMLObjectElement.
2)  getSVGDocument() on HTMLEmbedElement, HTMLIFrameElement, HTMLObjectElement

WebKit apparently also returns null from .frameElement if the caller does not subsume the element.  I would probably be fine with doing that too, actually.
Yes! This can only simplify things which is good. Though our existing infrastructure handles this situation quite well, you never know if we might be able to simplify things in the future.
Yeah I think the compartment mechanics here should be straightforward. For the Xray case, we should be in the caller's compartment, and so a simple CallerSubsumes-style check should do the trick here.

This has the remote possibility of breaking chrome code that waives Xray, inspects a content scope, and tries to work its way to a second (not-same-origin) content scope via contentDocument. But that's a pretty contrived example. I say do it to improve spec coherence here.
Blocks: 863933
Created attachment 744340 [details] [diff] [review]
Part 1 - Add convenience version of Equals/Subsumes to nsIPrincipal. v1
Attachment #744340 - Flags: review?(bzbarsky)
Created attachment 744341 [details] [diff] [review]
Part 2 - Return null for cross-origin contentDocument. v1
Attachment #744341 - Flags: review?(bzbarsky)
Created attachment 744342 [details] [diff] [review]
Part 3 - Tests. v1
Attachment #744342 - Flags: review?(bzbarsky)
https://tbpl.mozilla.org/?tree=Try&rev=826265e7cf35
Comment on attachment 744340 [details] [diff] [review]
Part 1 - Add convenience version of Equals/Subsumes to nsIPrincipal. v1

r=me
Attachment #744340 - Flags: review?(bzbarsky) → review+
Comment on attachment 744341 [details] [diff] [review]
Part 2 - Return null for cross-origin contentDocument. v1

> HTMLObjectElement::GetContentDocument(nsIDOMDocument **aContentDocument)

You don't need any changes to this method: it's calling the WebIDL one for the real work.

> nsGenericHTMLFrameElement::GetContentDocument(nsIDOMDocument** aContentDocument)

Likewise.

r=me with that.
Attachment #744341 - Flags: review?(bzbarsky) → review+
Comment on attachment 744342 [details] [diff] [review]
Part 3 - Tests. v1

r=me.  Want to toss some getSVGDocument() tests in there too?

Also, perhaps a followup on frameElement?
Attachment #744342 - Flags: review?(bzbarsky) → review+
Created attachment 744905 [details] [diff] [review]
Part 4 Fix up tests that depend on contentDocument being non-null. v1

These almost universally depend on some sort of special privileges, so I don't
think they're representative of any use-cases we might find on the web.
Attachment #744905 - Flags: review?(imelven)
https://tbpl.mozilla.org/?tree=Try&rev=6d9089068a2b
Blocks: 868235
(In reply to Boris Zbarsky (:bz) from comment #9)
> Also, perhaps a followup on frameElement?

Filed bug 829872.
Created attachment 745000 [details] [diff] [review]
Part 4 - Fix up tests that depend on contentDocument being non-null. v2

Fixed a misplaced paren. This is green with that change.
Attachment #744905 - Attachment is obsolete: true
Attachment #744905 - Flags: review?(imelven)
Attachment #745000 - Flags: review?(imelven)

Comment 14

4 years ago
Comment on attachment 745000 [details] [diff] [review]
Part 4 - Fix up tests that depend on contentDocument being non-null. v2

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

Changes make sense and look good to me, I am not a peer tho :)
Attachment #745000 - Flags: review?(imelven) → review+
(In reply to Ian Melven :imelven from comment #14)
> Changes make sense and look good to me, I am not a peer tho :)

That's ok. Mass test changes like this just need someone to understand the issue at hand.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/036cde16f9d5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/70cfbdceb63a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fb498cfb4058
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bfe5c0296c3b
https://hg.mozilla.org/mozilla-central/rev/036cde16f9d5
https://hg.mozilla.org/mozilla-central/rev/70cfbdceb63a
https://hg.mozilla.org/mozilla-central/rev/fb498cfb4058
https://hg.mozilla.org/mozilla-central/rev/bfe5c0296c3b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
I've added this bug to the compatibility doc. Please correct the info if wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_23
Keywords: dev-doc-needed, site-compat
OS: Mac OS X → All
Hardware: x86 → All

Updated

4 years ago
Duplicate of this bug: 528663

Comment 19

4 years ago
(In reply to Kohei Yoshino [:kohei] from comment #17)
> I've added this bug to the compatibility doc. Please correct the info if
> wrong.
> https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_23
Looks good to me.
Keywords: dev-doc-needed → dev-doc-complete

Updated

4 years ago
Depends on: 905624
Assignee: nobody → bobbyholley+bmo
You need to log in before you can comment on or make changes to this bug.