Last Comment Bug 829872 - Consider returning null from contentDocument getters when the caller does not subsume the document
: Consider returning null from contentDocument getters when the caller does not...
Status: RESOLVED FIXED
: dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla23
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
:
Mentors:
: 528663 (view as bug list)
Depends on: 905624
Blocks: 868235 CVE-2013-1687
  Show dependency treegraph
 
Reported: 2013-01-11 20:17 PST by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2013-10-10 12:16 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Add convenience version of Equals/Subsumes to nsIPrincipal. v1 (2.28 KB, patch)
2013-05-01 16:33 PDT, Bobby Holley (:bholley) (busy with Stylo)
bzbarsky: review+
Details | Diff | Splinter Review
Part 2 - Return null for cross-origin contentDocument. v1 (3.70 KB, patch)
2013-05-01 16:33 PDT, Bobby Holley (:bholley) (busy with Stylo)
bzbarsky: review+
Details | Diff | Splinter Review
Part 3 - Tests. v1 (3.82 KB, patch)
2013-05-01 16:34 PDT, Bobby Holley (:bholley) (busy with Stylo)
bzbarsky: review+
Details | Diff | Splinter Review
Part 4 Fix up tests that depend on contentDocument being non-null. v1 (17.33 KB, patch)
2013-05-02 16:40 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Part 4 - Fix up tests that depend on contentDocument being non-null. v2 (18.12 KB, patch)
2013-05-02 21:21 PDT, Bobby Holley (:bholley) (busy with Stylo)
ian.melven: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2013-01-11 20:17:09 PST
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 2013-01-12 16:37:44 PST
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) (busy with Stylo) 2013-01-13 14:55:05 PST
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) (busy with Stylo) 2013-05-01 16:33:42 PDT
Created attachment 744340 [details] [diff] [review]
Part 1 - Add convenience version of Equals/Subsumes to nsIPrincipal. v1
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2013-05-01 16:33:55 PDT
Created attachment 744341 [details] [diff] [review]
Part 2 - Return null for cross-origin contentDocument. v1
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2013-05-01 16:34:06 PDT
Created attachment 744342 [details] [diff] [review]
Part 3 - Tests. v1
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2013-05-01 16:35:42 PDT
https://tbpl.mozilla.org/?tree=Try&rev=826265e7cf35
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2013-05-02 12:22:19 PDT
Comment on attachment 744340 [details] [diff] [review]
Part 1 - Add convenience version of Equals/Subsumes to nsIPrincipal. v1

r=me
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2013-05-02 12:30:12 PDT
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 [:bz] (still a bit busy) 2013-05-02 12:36:37 PDT
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) (busy with Stylo) 2013-05-02 16:40:08 PDT
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.
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2013-05-02 16:55:52 PDT
https://tbpl.mozilla.org/?tree=Try&rev=6d9089068a2b
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2013-05-02 16:58:04 PDT
(In reply to Boris Zbarsky (:bz) from comment #9)
> Also, perhaps a followup on frameElement?

Filed bug 829872.
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2013-05-02 21:21:06 PDT
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.
Comment 14 Ian Melven :imelven 2013-05-03 14:09:21 PDT
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) (busy with Stylo) 2013-05-03 14:48:16 PDT
(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 17 Kohei Yoshino [:kohei] 2013-05-17 13:12:15 PDT
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 18 Jesse Ruderman 2013-06-09 18:34:42 PDT
*** Bug 528663 has been marked as a duplicate of this bug. ***
Comment 19 David Bruant 2013-06-18 07:38:10 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.