document.origin doesn't match spec

[Boris Zbarsky]

STEPS TO REPRODUCE:

1)  Load https://webkit.org
2)  Evaluate document.origin in the console.

EXPECTED RESULTS: Returns "https://webkit.org"

ACTUAL RESULTS: Returns "https_webkit.org_0

DETAILS: Spec is at https://dom.spec.whatwg.org/#dom-document-origin

[Chris Dumez]

I can reproduce. Thanks for the bug report. I'll take a look soon.

[Boris Zbarsky]

Note that there is a proposal from Blink to just remove document.origin altogether in favor of self.origin.

[Chris Dumez]

Created attachment 301208 [details]
WIP Patch

[Build Bot]

Comment on attachment 301208 [details]
WIP Patch

Attachment 301208 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3065179

New failing tests:
http/tests/media/media-stream/enumerate-devices-source-id-persistent.html

[Build Bot]

Created attachment 301214 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Boris Zbarsky]

Chris, I'm not sure whether you saw comment 2?

[Chris Dumez]

Created attachment 301237 [details]
Patch

[Chris Dumez]

(In reply to comment #6)
> Chris, I'm not sure whether you saw comment 2?

I saw. I don't think we would remove it short-term though. So for now, it is better to match the spec and other shipping browsers. If we end up removing it later on (after we implement self.origin), so be it.

[WebKit Commit Bot]

Comment on attachment 301237 [details]
Patch

Clearing flags on attachment: 301237

Committed r212178: <http://trac.webkit.org/changeset/212178>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This code was added in https://trac.webkit.org/changeset/164036, and it's really surprising if it wasn't breaking a whole lot of things besides the visible value of document.origin. Is it really not used anywhere else in the engine?

+PassRefPtr<Document> Document::create(ScriptExecutionContext& context)
+{
+    RefPtr<Document> document = adoptRef(new Document(nullptr, URL()));
+    document->setSecurityOrigin(context.securityOrigin());
+    return document;
+}
+

...
 
+String Document::origin() const
+{
+    return securityOrigin()->databaseIdentifier();
+}
+

[Chris Dumez]

(In reply to comment #11)
> This code was added in https://trac.webkit.org/changeset/164036, and it's
> really surprising if it wasn't breaking a whole lot of things besides the
> visible value of document.origin. Is it really not used anywhere else in the
> engine?
> 
> +PassRefPtr<Document> Document::create(ScriptExecutionContext& context)
> +{
> +    RefPtr<Document> document = adoptRef(new Document(nullptr, URL()));
> +    document->setSecurityOrigin(context.securityOrigin());
> +    return document;
> +}
> +
> 
> ...
>  
> +String Document::origin() const
> +{
> +    return securityOrigin()->databaseIdentifier();
> +}
> +

What do you mean by anywhere *else*. Document::origin() is not used *anywhere* in the engine and therefore my patch only changes the value exposed to the Web to match the specification and other browsers.

Our internal code relies on securityOrigin(), not origin().

[Alexey Proskuryakov]

As the diff shows, the original change was setting securityOrigin from origin, so both were incorrect.

[Alexey Proskuryakov]

Sorry, misread the original patch.