1139425 - Service Worker Client id should return a DOMString uuid

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Albert [:albert]]

According to specs service Worker Client id should return a DOMString uuid instead of unsigned long id.

https://github.com/slightlyoff/ServiceWorker/issues/634

Comment 1

[Albert [:albert]]

Attachment: Patch

Changed id type in webidl and added id string in ServiceWorkerClient

Comment 2

[Cătălin Badea (:catalinb)]

Comment on attachment 8573391 [details] [diff] [review]
Patch

lgtm

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8573391 [details] [diff] [review]
Patch

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

r=me but need dom peer sign off for webidl change.

::: dom/workers/ServiceWorkerClient.cpp
@@ +46,5 @@
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));
> +
> +  char buffer[NSID_LENGTH];
> +  id.ToProvidedString(buffer);
> +  mId = NS_ConvertUTF8toUTF16(Substring(buffer + 1, buffer + NSID_LENGTH - 2));

Add a comment about why you do this.

Comment 4

[Cătălin Badea (:catalinb)]

Comment on attachment 8573391 [details] [diff] [review]
Patch

Sorry for not noticing this earlier.

The id should uniquely identify the 'service worker client', which is defined as an environment settings object that has an associated worker and can be controlled by it. Therefore, client objects representing the same (controlled) document should have the same UUID.

http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#dfn-service-worker-client

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8573391 [details] [diff] [review]
Patch

Dropping baku's review. Catalin, please ask for it again after you've r+ed the patch.

Comment 6

[Albert [:albert]]

Attachment: Patch v2

Moved UUID generation to nsDocument

Comment 7

[Cătălin Badea (:catalinb)]

Comment on attachment 8576158 [details] [diff] [review]
Patch v2

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

The patch looks good, but I think you can keep the getter as a non virtual method in nsIDocument.

It would also help to add a test checking that the clients' UUID is persistent between different matchAll() calls.

Passing over to baku.

::: dom/base/nsDocument.cpp
@@ +12838,5 @@
>    return element.forget();
>  }
>  
> +void
> +nsDocument::GetId(nsAString& aId)

nsIDocument::GetId

@@ +12851,5 @@
> +
> +    char buffer[NSID_LENGTH];
> +    id.ToProvidedString(buffer);
> +
> +    mId = Substring(buffer + 1, buffer + NSID_LENGTH - 2);

Add comment for why the pointer arithmetic is needed.

@@ +12857,5 @@
> +
> +  CopyASCIItoUTF16(mId, aId);
> +}
> +
> +

Remove extra line.

::: dom/base/nsIDocument.h
@@ +748,5 @@
>    }
>  
> +  virtual void GetId(nsAString& aId) {
> +    CopyASCIItoUTF16(mId, aId);
> +  }

No need for the definition here and remove the virtual qualifier.

@@ +2787,5 @@
>  
>    // The channel that got passed to nsDocument::StartDocumentLoad(), if any.
>    nsCOMPtr<nsIChannel> mChannel;
> +
> +  nsCString mId;

Make this private.

::: dom/webidl/Document.webidl
@@ +347,5 @@
>    [ChromeOnly] readonly attribute nsIDocShell? docShell;
>  
>    [ChromeOnly] readonly attribute DOMString contentLanguage;
> +
> +  readonly attribute DOMString id;

This doesn't need to be exposed to the web.

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8576158 [details] [diff] [review]
Patch v2

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

Can you send me a new version of this patch with these comments? Thanks!

::: dom/base/nsDocument.cpp
@@ +226,5 @@
>  #include "nsLocation.h"
>  #include "mozilla/dom/FontFaceSet.h"
>  #include "mozilla/dom/BoxObject.h"
>  
> +#include "nsIUUIDGenerator.h"

can you move this into some nsI* header block?

@@ +12845,5 @@
> +    nsCOMPtr<nsIUUIDGenerator> uuidgen = do_GetService("@mozilla.org/uuid-generator;1");
> +    MOZ_ASSERT(uuidgen);
> +
> +    nsID id;
> +    nsresult rv = uuidgen->GenerateUUIDInPlace(&id);

MOZ_ALWAYS_TRUE(NS_SUCCEEDED(gUUIDGenerator->GenerateUUIDInPlace(&id)));
Othwerwise 'rv' is not used in non-debug builds.

@@ +12851,5 @@
> +
> +    char buffer[NSID_LENGTH];
> +    id.ToProvidedString(buffer);
> +
> +    mId = Substring(buffer + 1, buffer + NSID_LENGTH - 2);

What about:

nsresult
nsIDocument::GetId(nsAString& aId) ...

if (mId.IsEmpty()) {
  nsresult rv;
  nsCOMPtr<nsIUUIDGenerator> uuidgen = do_GetService("@mozilla.org/uuid-generator;1", &rv);
  if (NS_WARN_IF(NS_FAILED(rv))) {
    return rv;
  }

  nsID id;
  rv = uuidgen->GenerateUUIDInPlace(&id);
  if (NS_WARN_IF(NS_FAILED(rv))) {
    return rv;
  }

  // Build a string in {xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx} format
  char buffer[NSID_LENGTH];
  id.ToProvidedString(buffer);
  NS_ConvertASCIItoUTF16 uuid(buffer);

  // Remove {} and the null terminator
  mId.Assign(Substring(uuid, 1, NSID_LENGTH - 3));
}

...

::: dom/base/nsDocument.h
@@ +1583,5 @@
>                                    const nsAString* aTypeExtension) MOZ_OVERRIDE;
>  
>    static bool IsWebComponentsEnabled(JSContext* aCx, JSObject* aObject);
>  
> +  virtual void GetId(nsAString& aId) MOZ_OVERRIDE;

remove this.

::: dom/base/nsIDocument.h
@@ +748,5 @@
>    }
>  
> +  virtual void GetId(nsAString& aId) {
> +    CopyASCIItoUTF16(mId, aId);
> +  }

This should be what you implemented in nsDocument.{h,cpp}

Comment 9

[Albert [:albert]]

Attachment: Patch v3

Changes from comment 7

Comment 10

[Albert [:albert]]

Attachment: Patch v3

Changes from comment 8

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8576627 [details] [diff] [review]
Patch v3

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

::: dom/base/nsIDocument.h
@@ +748,5 @@
>    nsTArray<nsRefPtr<mozilla::dom::AnonymousContent>>& GetAnonymousContents() {
>      return mAnonymousContents;
>    }
>  
> +  nsresult GetId(nsAString& aId) {

Move the implementation in nsDocument.cpp as it was. My fault if my previous comment was not clear.
Just do here:

  nsresult GetId(nsAString& aId);

and in nsDocument.cpp:

nsresult
nsIDocument::GetId(nsAString& aId)
{
  ...
}

::: dom/webidl/Document.webidl
@@ +347,5 @@
>    [ChromeOnly] readonly attribute nsIDocShell? docShell;
>  
>    [ChromeOnly] readonly attribute DOMString contentLanguage;
> +
> +  [ChromeOnly] readonly attribute DOMString id;

We don't need this. Remove it.

::: dom/workers/ServiceWorkerClient.cpp
@@ +30,5 @@
>  {
>    MOZ_ASSERT(aDoc);
>    MOZ_ASSERT(aDoc->GetWindow());
>  
> +  aDoc->GetId(mClientId);

nsresult rv = aDoc->GetId(mChildId);
if (NS_FAILED(rv)) {
  NS_WARNING("Failed to get the UUID of the document."); // or a better message.
}

Comment 12

[Albert [:albert]]

Attachment: Patch v4

Comment 13

[Albert [:albert]]

Attachment: Tests

Tests for Client id

Comment 14

[Albert [:albert]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3e8b0995427

Comment 15

[Albert [:albert]]

Attachment: PAtch v4

Fixed return value for non-void function

Comment 16

[Albert [:albert]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2d382ebec15

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 8576800 [details] [diff] [review]
Tests

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

::: dom/workers/test/serviceworkers/match_all_client_id_worker.js
@@ +14,5 @@
> +      id[counter] = client.id;
> +      counter++;
> +      if (counter >= iterations) {
> +        var response = true;
> +        for (var index = 0; index < iterations; index++) {

index = 1

Comment 18

[Albert [:albert]]

Attachment: Patch - tests

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfa2d2fe0a8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/b60fe2874585

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dfa2d2fe0a8a
https://hg.mozilla.org/mozilla-central/rev/b60fe2874585