964148 - location.origin is not available in workers

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Kevin Grandon :kgrandon]

The location object is available in workers as read-only access. All properties seem to be present except the origin property. Furthermore, the mdn page states,  "origin on all location objects" is supported on version 26+.

Being that origin is missing from location objects in workers, I think we should add it.

Actual:
When accessed from a worker, location.origin is undefined.

Expected:
When accessed from a worker, location.origin should contain the expected protocol and host as found in the location object.

Comment 1

[Kevin Grandon :kgrandon]

This was just added in the spec: https://github.com/whatwg/url/commit/0027a2ec9d30501081da97b82455e07e67e22e2b

Comment 2

[Kevin Grandon :kgrandon]

Attachment: Patch - Add location.origin to workers

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8366278 [details] [diff] [review]
Patch - Add location.origin to workers

Review of attachment 8366278 [details] [diff] [review]:
-----------------------------------------------------------------

Can you add a mochitest?
I would like to review this patch again with these changes.
Thanks.

::: dom/workers/WorkerPrivate.cpp
@@ +3450,5 @@
> +  origin.Append(mLocationInfo.mProtocol);
> +  origin.Append("//");
> +  origin.Append(mLocationInfo.mHost);
> +
> +  mLocationInfo.mOrigin.Assign(origin);

you should use mOrigin to the LocationInfo struct and use

  nsContentUtils::GetUTFNonNullOrigin(aBaseURI, mLocationInfo.mOrigin);

This doesn't work for all the origin. Think about blob:, file: etc etc.

Comment 4

[Kevin Grandon :kgrandon]

Attachment: Patch - Add location.origin to workers

Hey Baku - thanks for the quick review. I've updated the patch.

One thing that's a bit new/odd to me is the casting. I've tried to follow examples elsewhere, and this seemed to be the most appropriate way to do it. Let me know if you think there is a better way, thanks!

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8366362 [details] [diff] [review]
Patch - Add location.origin to workers

Review of attachment 8366362 [details] [diff] [review]:
-----------------------------------------------------------------

Can you provide a mochitest? There are several examples in dom/workers/test/
I would create a test_origin.html that creates a worker with file_origin.js and here you should check if the origin is == to what you expect it should be.

::: dom/workers/WorkerPrivate.cpp
@@ +3445,5 @@
>    else {
>      mLocationInfo.mHost.Assign(mLocationInfo.mHostname);
>    }
> +
> +  NS_ConvertUTF8toUTF16 origin(mLocationInfo.mOrigin);

remove this and use:

nsCString origin;
nsContentUtils::GetUTFNonNullorigin(aBaseURI, origin);
mLocationInfo.mOrigin = NS_ConvertUTF16toUTF8(origin);

Comment 6

[Kevin Grandon :kgrandon]

Attachment: Patch - Add location.origin to workers

(In reply to Andrea Marchesini (:baku) from comment #5)
> Comment on attachment 8366362 [details] [diff] [review]
> Patch - Add location.origin to workers
> 
> Review of attachment 8366362 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you provide a mochitest? There are several examples in dom/workers/test/
> I would create a test_origin.html that creates a worker with file_origin.js
> and here you should check if the origin is == to what you expect it should
> be.
> 
> ::: dom/workers/WorkerPrivate.cpp
> @@ +3445,5 @@
> >    else {
> >      mLocationInfo.mHost.Assign(mLocationInfo.mHostname);
> >    }
> > +
> > +  NS_ConvertUTF8toUTF16 origin(mLocationInfo.mOrigin);
> 
> remove this and use:
> 
> nsCString origin;
> nsContentUtils::GetUTFNonNullorigin(aBaseURI, origin);
> mLocationInfo.mOrigin = NS_ConvertUTF16toUTF8(origin);

Hi Andrea, thanks for the quick review as always. I've had to change it a bit to use nsString origin, instead of nsCString - so please confirm that this is ok. (Get an error otherwise).

I have also made a small modification to the test_location mochitest as it seemed to be an appropriate place to test for origin. I am also going to run this on try to make sure everything passes there.

Comment 7

[Kevin Grandon :kgrandon]

On try: https://tbpl.mozilla.org/?tree=Try&rev=24b36ca08ab3

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8366379 [details] [diff] [review]
Patch - Add location.origin to workers

Review of attachment 8366379 [details] [diff] [review]:
-----------------------------------------------------------------

Bent, this patch looks good to me, but can you take a quick look?

::: dom/workers/WorkerPrivate.h
@@ +131,4 @@
>      nsCString mPathname;
>      nsCString mSearch;
>      nsCString mHash;
> +    nsCString mOrigin;

if this is a nsString, you don't hve to convert it twice. Change it to nsString and then do:

nsContentUtils::GetUTFNonNullOrigin(aBase, mLocation.mOrigin);

in WorkerPrivate.cpp.

And in Location.cpp, just use mOrigin instead NS_ConvertUTF8toUTF16(aInfo.mOrigin));

Comment 9

[Kevin Grandon :kgrandon]

(In reply to Andrea Marchesini (:baku) from comment #8)
> Comment on attachment 8366379 [details] [diff] [review]
> Patch - Add location.origin to workers
> 
> Review of attachment 8366379 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> if this is a nsString, you don't hve to convert it twice. Change it to
> nsString and then do:
> 
> nsContentUtils::GetUTFNonNullOrigin(aBase, mLocation.mOrigin);
> 
> in WorkerPrivate.cpp.
> 
> And in Location.cpp, just use mOrigin instead
> NS_ConvertUTF8toUTF16(aInfo.mOrigin));


Sure, will update - thanks! I was trying to follow the existing conventions in WorkerPrivate.h.

Comment 10

[Kevin Grandon :kgrandon]

Attachment: Patch - Add location.origin to workers

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8366711 [details] [diff] [review]
Patch - Add location.origin to workers

Review of attachment 8366711 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00470cecfc56

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00470cecfc56

Comment 14

[Jean-Yves Perrier [:teoli]]

Documentation updated:
https://developer.mozilla.org/en-US/docs/URLUtilsReadOnly.origin
https://developer.mozilla.org/en-US/docs/Web/API/URLUtilsReadOnly
https://developer.mozilla.org/en-US/docs/Web/API/WorkerLocation
and
https://developer.mozilla.org/en-US/Firefox/Releases/29