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

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 2

[Bobby Holley (:bholley)]

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.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add convenience version of Equals/Subsumes to nsIPrincipal. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Return null for cross-origin contentDocument. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Tests. v1

Comment 6

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=826265e7cf35

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 744340 [details] [diff] [review]
Part 1 - Add convenience version of Equals/Subsumes to nsIPrincipal. v1

r=me

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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?

Comment 10

[Bobby Holley (:bholley)]

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

Comment 11

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=6d9089068a2b

Comment 12

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #9)
> Also, perhaps a followup on frameElement?

Filed bug 829872.

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 4 - Fix up tests that depend on contentDocument being non-null. v2

Fixed a misplaced paren. This is green with that change.

Comment 14

[Ian Melven :imelven]

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

Comment 15

[Bobby Holley (:bholley)]

(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

Comment 16

[Phil Ringnalda (:philor)]

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

Comment 17

[Kohei Yoshino [:kohei]]

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

Comment 19

[David Bruant]

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