Closed Bug 1139425 Opened 5 years ago Closed 5 years ago

Service Worker Client id should return a DOMString uuid

Categories

(Core :: DOM: Workers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: albert, Assigned: albert)

References

Details

Attachments

(2 files, 6 obsolete files)

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

https://github.com/slightlyoff/ServiceWorker/issues/634
Assignee: nobody → alberto.crespellperez
Attached patch Patch (obsolete) — Splinter Review
Changed id type in webidl and added id string in ServiceWorkerClient
Attachment #8573391 - Flags: feedback?(catalin.badea392)
Comment on attachment 8573391 [details] [diff] [review]
Patch

lgtm
Attachment #8573391 - Flags: feedback?(catalin.badea392) → feedback+
Attachment #8573391 - Flags: review?(nsm.nikhil)
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.
Attachment #8573391 - Flags: review?(nsm.nikhil) → review?(amarchesini)
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
Attachment #8573391 - Flags: feedback+ → feedback-
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.
Attachment #8573391 - Flags: review?(amarchesini)
Status: NEW → ASSIGNED
Attached patch Patch v2 (obsolete) — Splinter Review
Moved UUID generation to nsDocument
Attachment #8573391 - Attachment is obsolete: true
Attachment #8576158 - Flags: feedback?(catalin.badea392)
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.
Attachment #8576158 - Flags: review?(amarchesini)
Attachment #8576158 - Flags: feedback?(catalin.badea392)
Attachment #8576158 - Flags: feedback+
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}
Attachment #8576158 - Flags: review?(amarchesini) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
Changes from comment 7
Attachment #8576158 - Attachment is obsolete: true
Attachment #8576601 - Flags: review?(amarchesini)
Attachment #8576601 - Flags: review?(amarchesini)
Attached patch Patch v3 (obsolete) — Splinter Review
Changes from comment 8
Attachment #8576601 - Attachment is obsolete: true
Attachment #8576627 - Flags: review?(amarchesini)
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.
}
Attachment #8576627 - Flags: review?(amarchesini) → review+
Attached patch Patch v4 (obsolete) — Splinter Review
Attachment #8576627 - Attachment is obsolete: true
Attachment #8576692 - Flags: review+
Attached patch Tests (obsolete) — Splinter Review
Tests for Client id
Attachment #8576800 - Flags: review?(amarchesini)
Attached patch PAtch v4Splinter Review
Fixed return value for non-void function
Attachment #8576692 - Attachment is obsolete: true
Attachment #8577092 - Flags: review+
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
Attachment #8576800 - Flags: review?(amarchesini) → review+
Attached patch Patch - testsSplinter Review
Attachment #8576800 - Attachment is obsolete: true
Attachment #8577965 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dfa2d2fe0a8a
https://hg.mozilla.org/mozilla-central/rev/b60fe2874585
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.