Closed Bug 1493860 Opened 6 years ago Closed 5 years ago

Audit the IDLs where we are now defaulting dictionary-typed members of dictionaries to null

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: marcosc)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-needed)

In bug 1368949 I am going to change the behavior of our webidl codegen for optional dictionary-typed members of dictionaries to allow them to be "not present" instead of always defaulting to null. I am adding explicit "= null" to all of the current uses to preserve behavior; the change is being made now so some new APIs can rely on it. This bug is for going through those "= null" and deciding which of them should stay (and get upstreamed to the relevant specs) and which should go away. In the latter case, the relevant specs should be audited to see whether they define the "not present" case. We may want multiple bugs under this one tracking the various spec bits.
Depends on: 1368949
For DeviceMotionEventInit, there is some ongoing discussion at https://github.com/w3c/deviceorientation/issues/54
Consequences of "null becomes empty dictionary." Will do these incrementally... starting with the easy one. ## Payment Request With: ``` PaymentDetailsModifier{ PaymentItem total = null; } ``` Would break, `PaymentItem` has `required` members `currency` and `item`. Those members can't be defaulted, unfortunately. ## dictionary PaymentDetailsUpdate With the following change: ``` dictionary PaymentDetailsUpdate : PaymentDetailsBase { AddressErrors shippingAddressErrors = null; PaymentItem total = null; } ``` Setting `shippingAddressErrors` to null is fine, however. However, setting `total` to null would break for the reasons give above about `PaymentItem`.
Priority: -- → P2
Priority: P2 → P1
## Audit of Web Authentication / Credential Management 1) Should the "= null" stay? This is bug: https://github.com/w3c/webauthn/issues/750 And yes, it should stay. Also with the fixes in #750. The spec needs to be updated to deal with "challenge" being null, as well as some other values being nulled. Git blame says ask @qdot :) What happens if `mChallenge` is passed as null in `dom/webauthn/WebAuthnManager.cpp`? I'm guessing it's all good, as you reviewed bug 1368949?
Flags: needinfo?(kyle)
## Audit of Push API The IDL is out of date! The proposed change was: ``` dictionary PushSubscriptionJSON { PushSubscriptionKeys keys = null; } ``` In the latest spec, it is now: ``` dictionary PushSubscriptionJSON { record<DOMString, USVString> keys; } ``` This one doesn't matter. It's the result of a PushSubscription.toJSON(). @lina, I couldn't find a bug to update the IDL to match latest spec. Do you have any status on Push API implementation?
Flags: needinfo?(lina)
I've sent a pull request to Payment Request to default the *Error members ones to `null`. https://github.com/w3c/payment-request/pull/781 I've checked the algorithms in the spec, and they are all fine (basically "For each member, show error..." - but no requirement that any member be there). We now need to deal explicitly with `total`s PaymentCurrencyAmount... I don't think suggesting defaults to the Working Group for `currency` and `value` wouldn't got down well.
So one complication: it's not clear to me that the "= null" thing is valid IDL given the current state of the IDL spec. It works in Gecko, and I think having it work in the IDL spec would make some sense, but it's not clear that it does right now. For WebAuthn, I'm not sure whether they decided to allow a missing challenge or to stick with the challenge being required. Needs to get sorted out... For payment request, if the spec algorithms handle the "not present" case, why would we want to add "= null"? What's the state of the MediaStreamTrack bit?
Bringing in :jcj since he's dealing with the WebAuthn spec, which has changed a lot since I did my work there.
Flags: needinfo?(kyle)
Flags: needinfo?(jjones)
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #6) > So one complication: it's not clear to me that the "= null" thing is valid > IDL given the current state of the IDL spec. It works in Gecko, and I think > having it work in the IDL spec would make some sense, but it's not clear > that it does right now. > > For WebAuthn, I'm not sure whether they decided to allow a missing challenge > or to stick with the challenge being required. Needs to get sorted out... My understanding of the WebAuth issue is that null is a valid Buffer value. So even requiring it, a developer can legally pass null (and the spec doesn’t seem to deal with that). > For payment request, if the spec algorithms handle the "not present" case, > why would we want to add "= null"? If null becomes an empty dictionary, then it works just fine because it means one less check. But yes, checking “is present” works, then walk the members that are present (if any). > What's the state of the MediaStreamTrack bit? Sorry, I saw the RTC folks so involved in the other bugs that I figured they had a solution. I’ll deep dive into that too.
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #6) > For WebAuthn, I'm not sure whether they decided to allow a missing challenge > or to stick with the challenge being required. Needs to get sorted out... It's definitely required, may not be null [0], and if we reach one of the places we copy Challenge [1][2] then badness could ensuse. Best case scenario is an untrustworthy webauthn session. Doing this on current nightly, presumably with bug 1368949, still yields > TypeError: Missing required 'challenge' member of PublicKeyCredentialRequestOptions. but I would guess we need to check each parameter on the way into the function now rather than rely on the webidl checks? Regarding the challenge field in particular, I filed Bug 1494738 to do something for the last sentence of [0]. [0] https://w3c.github.io/webauthn/#cryptographic-challenges [1] https://searchfox.org/mozilla-central/source/dom/webauthn/WebAuthnManager.cpp#331 [2] https://searchfox.org/mozilla-central/source/dom/webauthn/WebAuthnManager.cpp#480
Flags: needinfo?(jjones)
> Doing this on current nightly, presumably with bug 1368949, still yields Right, because I explicitly kept the old behavior pending someone who knows webauthn looking at it. What you can do is remove the "= null" bits from WebAuthentication.webidl and then fix the resulting compile errors in whatever way the spec says that stuff should work. Assuming it says.
Depends on: 1497111
:jib, I could use some help with the WebRTC parts - please see what changed in the IDL in 1368949.
Flags: needinfo?(jib)
Thanks, I've opened bug 1497351 to fix the WebRTC parts.
Flags: needinfo?(jib)
Depends on: 1497385
(In reply to Marcos Caceres [:marcosc] from comment #4) > @lina, I couldn't find a bug to update the IDL to match latest spec. Do you > have any status on Push API implementation? Hi Marcos, I'm so sorry for the delay in getting back to you! I filed bug 1497427 to capture the work to update the Push API; bug 1497429, bug 1497430, and bug 1497431 are for the IDL bits. Unfortunately, I don't have cycles to work on them. :-(
Flags: needinfo?(lina)
Component: DOM → DOM: Core & HTML
Priority: P1 → P2

The remaining bug for Push is tracked by https://bugzilla.mozilla.org/show_bug.cgi?id=1497427, so closing this.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.