Bug 115643

Summary: Document should be constructable
Product: WebKit Reporter: Erik Arvidsson <arv>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, esprehn+autocc, kangil.han, kondapallykalyan, llango.u-szeged, philip, rniwa, syoichi
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=168022
Bug Depends on:    
Bug Blocks: 115701    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Erik Arvidsson 2013-05-06 07:31:58 PDT
http://dom.spec.whatwg.org/#document

Make Document constructable so that one can do "new Document" instead of "document.implementation.createHTMLDocument('')"
Comment 1 László Langó 2014-01-30 01:09:02 PST
Created attachment 222642 [details]
Patch
Comment 2 Darin Adler 2014-01-30 17:22:44 PST
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.
Comment 3 Darin Adler 2014-01-30 17:24:55 PST
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.
Comment 4 László Langó 2014-02-05 07:09:59 PST
(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
Comment 5 Darin Adler 2014-02-07 11:14:36 PST
(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.
Comment 6 László Langó 2014-02-12 04:29:45 PST
Created attachment 223958 [details]
Patch
Comment 7 Darin Adler 2014-02-12 19:36:12 PST
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.
Comment 8 László Langó 2014-02-13 02:31:36 PST
(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
Comment 9 László Langó 2014-02-13 02:35:20 PST
Created attachment 224054 [details]
Patch
Comment 10 Darin Adler 2014-02-13 09:49:12 PST
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.
Comment 11 WebKit Commit Bot 2014-02-13 10:26:40 PST
Comment on attachment 224054 [details]
Patch

Clearing flags on attachment: 224054

Committed r164036: <http://trac.webkit.org/changeset/164036>
Comment 12 WebKit Commit Bot 2014-02-13 10:26:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 László Langó 2014-02-14 03:50:05 PST
(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
Comment 14 Philip Jägenstedt 2017-02-14 22:14:04 PST
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
Comment 15 Chris Dumez 2017-02-14 22:30:46 PST
(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.