Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PushSubscriptionOptions ArrayBuffer lifetime issues #198

Closed
johnmellor opened this issue Jul 5, 2016 · 4 comments
Closed

PushSubscriptionOptions ArrayBuffer lifetime issues #198

johnmellor opened this issue Jul 5, 2016 · 4 comments

Comments

@johnmellor
Copy link
Contributor

PushSubscriptionOptions is spec'd as:

interface PushSubscriptionOptions {
    readonly        attribute boolean      userVisibleOnly;
    [Throws]
    readonly        attribute ArrayBuffer? applicationServerKey;
};
  1. What does [Throws] do? I couldn't find it in WebIDL - is it Mozilla-specific IDL syntax?
  2. Should the ArrayBuffer be marked [SameObject] since it never changes to allow == comparison?
  3. What happens if the developer detaches the ArrayBuffer by passing it in the transfer argument to postMessage (or using ArrayBuffer.transfer())? Do we need to freeze it or something like that? Would freezing even have any effect, since the data being detached is stored in an internal slot? See also w3c issue 23369. For example I noticed that @annevk used [NewObject] for https://encoding.spec.whatwg.org/#textencoder, but we probably can't use that here since [NewObject] is only for methods, not for attributes.

@martinthomson @beverloo

@annevk
Copy link
Member

annevk commented Jul 6, 2016

  1. Yeah, that should just be removed.
  2. Probably, yes.
  3. Freezing wouldn't do anything here. If the ArrayBuffer gets detached it would no longer be usable here. I don't really know what the processing model is for PushSubscriptionOptions objects so I can't really comment as to whether that is problematic.

@ghost
Copy link

ghost commented Jul 6, 2016

I was wondering if [SameObject] means scripts could mutate the ArrayBuffer, and how it behaves with two PushSubscription objects for the same scope. For example:

serviceWorkerRegistration.pushManager.getSubscription().then(subscription => {
  let applicationServerKey = subscription.options.applicationServerKey;

  let keyArray = new Uint8Array(applicationServerKey);
  keyArray[0] = 123;

  let maybeModifiedKey = subscription.options.applicationServerKey;

  // If `applicationServerKey` is `[SameObject]`, does that mean scripts could
  // change the contents of the key?
  assert(applicationServerKey == modifiedKey);
  assert(new Uint8Array(maybeModifiedKey)[0] == 123);

  return serviceWorkerRegistration.pushManager.getSubscription().then(newSubscription => {
    // `getSubscription` resolves to a new `PushSubscription` instance, so I assume
    // these are all true. Is that right?
    assert(newSubscription != subscription);
    assert(newSubscription.applicationServerKey != applicationServerKey);
    assert(new Uint8Array(newSubscription.applicationServerKey)[0] != 123);
  });
});

("Don't do that" is an acceptable answer for the first, but I'm curious how specs handle this kind of thing).

@annevk
Copy link
Member

annevk commented Jul 6, 2016

[SameObject] means that the IDL object effectively holds a pointer to the other object and keeps returning that.

The other object being mutated is immaterial to that.

If you want to support multiple consumers that cannot accidentally confuse each other you would need to use a method instead that returns copies from some internal state.

@johnmellor
Copy link
Contributor Author

The serviceWorkerRegistration.pushManager.subscribe(...) and getSubscription() promises are always resolved with a new PushSubscription object containing a snapshot of the subscription at that point in time. This in turn owns a [SameObject] readonly attribute PushSubscriptionOptions options.

Since PushSubscription objects are thus somewhat ephemeral, it actually seems pretty reasonable to allow the author to mutate the applicationServerKey or post it to a worker without copying.

Thanks all - I'll send out a PR to replace [Throws] with [SameObject].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants