Bug 115643 – Document should be constructable

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Erik Arvidsson]

http://dom.spec.whatwg.org/#document

Make Document constructable so that one can do "new Document" instead of "document.implementation.createHTMLDocument('')"

[László Langó]

Created attachment 222642 [details]
Patch

[Darin Adler]

Comment on attachment 222642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222642&action=review

> Source/WebCore/dom/Document.h:262
> +        return adoptRef(new Document(0, context.url()));

Please use nullptr instead of 0 here.

I don’t see why context.url() would be correct for this document. I think we need to make a document without a URL here.

DOMImplementation.createDocument also sets the security origin on the document it creates. I suspect we need to do the same here.

I think we need some test coverage that goes beyond “this is a document” and checks these other properties of the document we create.

[Darin Adler]

The things we need to test are the things that the standard requires:

- The document has an origin which is an alias to the origin of the global object's associated document. Need to test that either directly or indirectly.

- The document has an effective script origin which is an alias to the effective script origin of the global object's associated document. Need to test that either directly or indirectly.

[László Langó]

(In reply to comment #3)
> The things we need to test are the things that the standard requires:
> 
> - The document has an origin which is an alias to the origin of the global object's associated document. Need to test that either directly or indirectly.
> 
> - The document has an effective script origin which is an alias to the effective script origin of the global object's associated document. Need to test that either directly or indirectly.

Thanks for your comments. Can you help me a bit more with testing? I'm not sure how to test these correctly. Should we check these steps? http://www.whatwg.org/specs/web-apps/current-work/multipage/origin-0.html#sandboxOrigin

[Darin Adler]

(In reply to comment #4)
> (In reply to comment #3)
> > The things we need to test are the things that the standard requires:
> > 
> > - The document has an origin which is an alias to the origin of the global object's associated document. Need to test that either directly or indirectly.
> > 
> > - The document has an effective script origin which is an alias to the effective script origin of the global object's associated document. Need to test that either directly or indirectly.
> 
> Thanks for your comments. Can you help me a bit more with testing? I'm not sure how to test these correctly. Should we check these steps? http://www.whatwg.org/specs/web-apps/current-work/multipage/origin-0.html#sandboxOrigin

No, those steps are the algorithm the engine is supposed to follow. But a test needs to be based on the observable results of the algorithm, not the algorithm itself.

We need to figure out how to detect if a document has an incorrect origin or an incorrect effective script origin by the behavior of the engine.

I presume that the effect of the effective script origin is to restrict script execution, so that test would be about correct restricting script execution and not incorrectly restricting it.

The other test could perhaps be done simply by reading a property that indicates what a document’s origin is. I’m not really sure, because I’m not an expert on this part of the specification. I’d have to study it myself to figure that out.

[László Langó]

Created attachment 223958 [details]
Patch

[Darin Adler]

Comment on attachment 223958 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223958&action=review

Does the test fail if you remove the line of code that says setSecurityOrigin?

> Source/WebCore/dom/Document.cpp:565
> +    document->setSecurityOrigin(adoptRef(context.securityOrigin()));

The adoptRef here is incorrect. This will result in a reference count underflow.

[László Langó]

(In reply to comment #7)
> (From update of attachment 223958 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223958&action=review
> 
> Does the test fail if you remove the line of code that says setSecurityOrigin?

yes

[László Langó]

Created attachment 224054 [details]
Patch

[Darin Adler]

Comment on attachment 224054 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224054&action=review

> Source/WebCore/dom/Document.cpp:566
> +    return document;

Oh, I just realized this line should read document.release() to avoid a tiny bit of needless reference count churn. Probably will let this go into the commit queue anyway. We can follow up with a 1-line patch to fix that.

[WebKit Commit Bot]

Comment on attachment 224054 [details]
Patch

Clearing flags on attachment: 224054

Committed r164036: <http://trac.webkit.org/changeset/164036>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[László Langó]

(In reply to comment #10)
> (From update of attachment 224054 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224054&action=review
> 
> > Source/WebCore/dom/Document.cpp:566
> > +    return document;
> 
> Oh, I just realized this line should read document.release() to avoid a tiny bit of needless reference count churn. Probably will let this go into the commit queue anyway. We can follow up with a 1-line patch to fix that.

It's fixed. Committed in https://trac.webkit.org/r164099

[Philip Jägenstedt]

This change added document.origin to WebKit. There's a discussion on blink-dev about removing it from Blink in favor of self.origin that may be of interest:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/CO52Bt15cuc/ya6u_devCQAJ

[Chris Dumez]

(In reply to comment #14)
> This change added document.origin to WebKit. There's a discussion on
> blink-dev about removing it from Blink in favor of self.origin that may be
> of interest:
> https://groups.google.com/a/chromium.org/d/msg/blink-dev/CO52Bt15cuc/
> ya6u_devCQAJ

Well, we don't implement self.origin, so I do not think we can get rid of document.origin anytime soon. Also note that we recently aligned our implementation of document.origin with the specification and Blink.