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

Bug 29514 - Need to define which globals objects created by this spec are associated with #85

Closed
mwatson2 opened this issue May 24, 2016 · 32 comments
Assignees

Comments

@mwatson2
Copy link
Collaborator

Bug 29514:

Consider the attached testcase. I believe it should log false, true, false, true. That's not what I see in either Chrome or Firefox, though, and Chrome and Firefox don't agree with each other either.

The spec doesn't actually define behavior here. It simply says:

Let result be the CryptoKey object that results from performing the
import key operation specified by normalizedAlgorithm using keyData,
algorithm, format, extractable and usages.

and the specific algorithm sections say things like:

Let key be a new CryptoKey object that represents the RSA public key identified
by publicKey

without saying which global it's associated with. Note that the IDL specification requires all IDL objects to be explicitly associated with some global. There are proposals for adding some sort of implicit association for simple cases of synchronous object creation (which would make the first two things logged in this testcase be "false, true", as in Firefox), but async things would need to be handled a bit more explicitly still.

In any case, I consider the Firefox behavior here where the Promise and the CryptoKey come from different globals a bug (tracked at https://bugzilla.mozilla.org/show_bug.cgi?id=1250930) and will suggest that Firefox switch to the "false, true, false, true" behavior I mention above...

@mwatson2
Copy link
Collaborator Author

Here is the test-case:

<!DOCTYPE html>
<html>
<iframe></iframe>
<script>
  onload = function() {
    var p = crypto.subtle.importKey.call(
      frames[0].crypto.subtle,
      "raw",
      crypto.getRandomValues(new Uint8Array(10)),
      {name: "HMAC",hash: {name: "SHA-1"},},
      false,
      ["sign", "verify"]);
    console.log(p.__proto__ == Promise.prototype);
    console.log(p.__proto__ == frames[0].Promise.prototype);
    p.then(function(key) {
      console.log(key.__proto__ == CryptoKey.prototype);
      console.log(key.__proto__ == frames[0].CryptoKey.prototype)
    });
  }
</script>
</html>

@mwatson2
Copy link
Collaborator Author

@bzbarsky Have the "proposals for adding some sort of implicit association for simple cases of synchronous object creation" advanced since you filed this ?

In any case, could you point me at those proposals and / or suggest appropriate wording to resolve this ? Thanks!

@bzbarsky
Copy link

I don't know and I'm not sure where they live at the moment. Maybe check with @annevk?

@annevk
Copy link
Member

annevk commented May 26, 2016

@mwatson2
Copy link
Collaborator Author

mwatson2 commented Jul 11, 2016

May be solved by fixes for #87 and #88

@mwatson2
Copy link
Collaborator Author

I don't see any progress on the WebIDL bugs.

@bzbarsky @annevk Can you provide a form of words that we could use to at least improve on the current situation for WebCrypto. We create new Promise, CryptoKey, CryptoKeyPair and ArrayBuffer objects. I imagine a single statement of the following form:

All new JavaScript objects created by the methods defined in this specification shall be associated with the XXX global object.

What should XXX be ?

@bzbarsky
Copy link

You probably want to use either the "relevant global object of this" or the "current global object" depending on which behavior you want in which cases. See https://html.spec.whatwg.org/multipage/webappapis.html#current and https://html.spec.whatwg.org/multipage/webappapis.html#relevant and the explanations at https://html.spec.whatwg.org/multipage/webappapis.html#concept-current-everything and https://html.spec.whatwg.org/multipage/webappapis.html#concept-relevant-everything

@mwatson2
Copy link
Collaborator Author

mwatson2 commented Jul 12, 2016

@bzbarsky Thanks. Am I right that to have the test above output "false, true, false, true" we should use the "relevant global object of this" ? (that being the global associated with frames[0].crypto.subtle in the test)

@bzbarsky
Copy link

Yes, if you want "false, true, false, true" then you want the "relevant global object of this" for both the Promise and the CryptoKey.

@mwatson2
Copy link
Collaborator Author

Thanks. This next may be a stupid question, but for an ArrayBuffer - where we are returning the result of transforming the contents of some input ArrayBuffer somehow - should we do the same thing, or should the new ArrayBuffer have the same global object as the input one ?

@bzbarsky
Copy link

I don't have strong feelings on that, honestly.

@mwatson2
Copy link
Collaborator Author

Is anyone aware of any prior examples of functions with input / output objects, where the output is the result of processing the input ? I would be happy to follow past practice.

I looked at the Encoding specification, but this says nothing about the globals associated with the objects it returns.

I wonder if we are holding WebCrypto to a higher standard than other specifications here ? Or, at least, that this is a newly discovered source of ambiguity and we should wait for a general approach to be agreed (for example in WebIDL) before closing the ambiguity ?

Also, I'd note that the fact that the relevant WebIDL bugs have been open and inactive for quite some time does suggest this ambiguity (and the resulting potential difference in implementations) is not really a big problem for anyone.

@bzbarsky
Copy link

I looked at the Encoding specification, but this says nothing about the globals associated with the objects it returns.

Indeed. It should. Not that it doesn't have an object argument, so wouldn't have the option of "create the return value in the same global as the argument" to start with.

Or, at least, that this is a newly discovered source of ambiguity

Well, newly as in a few years ago.

I've had a very hard time convincing spec editors that they should address this. Then again, I've had a pretty hard time convincing them about the value of edge-case interop in general. :(

@mwatson2
Copy link
Collaborator Author

I've had a very hard time convincing spec editors that they should address this. Then again, I've had a pretty hard time convincing them about the value of edge-case interop in general. :(

I'm sympathetic, but speaking as a spec editor I don't have a lot to go on to make decisions like the one required here. In fact my decision would be arbitrary, if forced to make a choice.

It would be better if WebIDL could provide default behaviors that specs just inherit unless they specify otherwise. The decision may still be arbitrary, but at least it would consistent across lots of specifications.

@bzbarsky
Copy link

The default behavior being proposed in IDL is to create everything using the relevant global of this, I believe.

@mwatson2
Copy link
Collaborator Author

Ok, so then what I suggest for this specification is just to add a non-normative note as follows:

The global object to be associated with new objects returned by the method of this specification is expected to be defined in [WebIDL]. In the absence of such a definition it is recommended that implementations associate newly created objects with the relevant global object of this.

@bzbarsky
Copy link

I don't see why that's any better than just making this a normative requirement.

@mwatson2
Copy link
Collaborator Author

If, by any chance, the WebIDL team decided on a different behavior, then that would become the normative requirement and browsers would be able to implement that consistently across APIs, without worrying about WebCrypto being the one spec which defines something different. Things would also be more consistent (eventually) for authors.

@bzbarsky
Copy link

If, by any chance, the WebIDL team decided on a different behavior

Then I would assume we would update webcrypto accordingly.

@mwatson2
Copy link
Collaborator Author

Then I would assume we would update webcrypto accordingly.

We're driving towards PR / REC fairly soon, after which there is as yet no plan for a future version or a living spec. I suppose we could at least publish errata.

In this case, we expect that the problem will soon be taken out of our hands, so I thought it would be better to recognize that and avoid the possibility that WebCrypto and WebIDL contain conflicting normative requirements, even for a time.

@annevk
Copy link
Member

annevk commented Jul 18, 2016

If you don't update the spec someone else will have to... The idea you can just stop maintenance is bananas.

@mwatson2
Copy link
Collaborator Author

Things that are bananas seem to be happening with increasing frequency these days ...

Anyway, if we specify this normatively in WebCrypto,I would at least like to note that WebIDL may specific this in future. Ideally, WebIDL would specify this before we are done with WebCrypto. Is that likely ?

@bzbarsky
Copy link

I suppose we could at least publish errata.

Indeed. This is one of those cases where the W3C process is part of the problem, not part of the solution. (Of course in other cases, it is part of the solution....)

Is that likely ?

I'd guess not. IDL is starved for editing resources, unfortunately.

@mwatson2
Copy link
Collaborator Author

I'd guess not. IDL is starved for editing resources, unfortunately.

Would it help if I made a PR ? Or is the problem that there are insufficient people active to make a decision on what is the correct solution ?

@bzbarsky
Copy link

The latter. It doesn't help that making this decision needs some input from people working on the various UAs and their binding/DOM/etc implementations, and getting such input on IDL issues has been nearly impossible for years now. :(

@bzbarsky
Copy link

As in, what the editor needs to do in this case is not "write the spec" but "check with all the implementors, including the ones that don't answer their e-mail, whether they're OK with the spec".

@domenic
Copy link

domenic commented Jul 18, 2016

As someone else who's been involved in the multi-globals business from the spec-editing side, I'd just like to second @bzbarsky here. It would be very good to be precise about this in Web Crypto. I don't see Web IDL helping with this any time soon; in many cases it's something a spec needs to determine itself.

@mwatson2
Copy link
Collaborator Author

mwatson2 commented Sep 8, 2016

@ericroman920, @sleevi, @bzbarsky: Please see #135.

@bzbarsky
Copy link

bzbarsky commented Sep 9, 2016

@mwatson2 Is the change to spec/Overview-WebCryptoAPI.xml the only one you want me to look at? As in, I can ignore the .html changes?

If so, this is probably OK enough. Hard to find from the actual places that construct objects, so I expect UA implemetors to mess it up, but we can add tests....

@mwatson2
Copy link
Collaborator Author

mwatson2 commented Sep 9, 2016

Is the change to spec/Overview-WebCryptoAPI.xml the only one you want me to look at? As in, I can ignore the .html changes?

Yes, the html is auto-generated from the xml. We should move this into a TravisCI task, but haven't got round to that yet.

Hard to find from the actual places that construct objects

I could add it to all those places too.

@bzbarsky
Copy link

bzbarsky commented Sep 9, 2016

I could add it to all those places too.

I think that would make it harder to miss, for sure.

@mwatson2
Copy link
Collaborator Author

Ok, I've made this explicit everywhere we create an object (including specifying the return type in the procedures where it was missing). See #135.

@mwatson2 mwatson2 self-assigned this Sep 12, 2016
hhalpin pushed a commit that referenced this issue Oct 24, 2016
Fix #85: Specify the global object associated with objects we create
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants