1492737 - Fix the failure of keypath-exceptions.htm in web platform tests

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect, P2)

Comment 1

[Jan Varga [:janv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b30e43fe5a2f7a1a5662d7a4fb08047878577e3

Comment 2

[Jan Varga [:janv]]

Attachment: Part 1: Support passing name length to JS_GetOwnUCPropertyDescriptor and add JS_GetUCPropertyDescriptor

Comment 3

[Jan Varga [:janv]]

Attachment: Part 2: Avoid searching the prototype chain when extracting keys

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 9010623 [details] [diff] [review]
Part 1: Support passing name length to JS_GetOwnUCPropertyDescriptor and add JS_GetUCPropertyDescriptor

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

::: dom/xbl/nsXBLProtoImpl.cpp
@@ +97,5 @@
>    // holder called |foo| in the XBL scope. Check for that to avoid wasteful and
>    // weird property holder duplication.
> +  const nsString& className = aPrototypeBinding->ClassName();
> +  const char16_t* classNameChars = className.get();
> +  const size_t classNameLen = className.Length();

Put a blank line after this, give the code some breathing room.

::: js/src/jsapi.cpp
@@ +2083,3 @@
>                                MutableHandle<PropertyDescriptor> desc)
>  {
> +    JSAtom* atom = AtomizeChars(cx, name, AUTO_NAMELEN(name, namelen));

Don't do the AUTO_NAMELEN junk.  If users want to do that, they should do it themselves -- double-meaning parameters tend to gunk things up over time.  Just use namelen directly.  (And I guess because code lower down does want AUTO_NAMELEN -- and it is not necessary or critical to fix that now -- just leave it where it was.)

@@ +2113,5 @@
> +JS_PUBLIC_API(bool)
> +JS_GetUCPropertyDescriptor(JSContext* cx, HandleObject obj, const char16_t* name, size_t namelen,
> +                           MutableHandle<PropertyDescriptor> desc)
> +{
> +    JSAtom* atom = AtomizeChars(cx, name, AUTO_NAMELEN(name, namelen));

Again, use namelen directly.

Comment 5

[Jan Varga [:janv]]

Attachment: Part 1: Support passing name length to JS_GetOwnUCPropertyDescriptor and add JS_GetUCPropertyDescriptor

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9010624 [details] [diff] [review]
Part 2: Avoid searching the prototype chain when extracting keys

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

And I see from the keypath.htm that we definitely do have coverage for the Array.length and String.length special-cases also covered by the spec on key-path evaluation.  I infer the difference for Blob/File is that the WebIDL bindings are explicitly implemented as getters, so our logic, of necessity, must manually special-case these getters (consistent with the spec).

::: dom/indexedDB/KeyPath.cpp
@@ +166,5 @@
> +                  JS_NewUCStringCopyN(aCx, name.get(), name.Length());
> +
> +                intermediate = JS::StringValue(string);
> +                hasProp = true;
> +              } else if (token.EqualsLiteral("lastModified")) {

It might be worth calling out in a comment that although the spec also lists "lastModifiedDate" that we deprecated and removed support for it.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96c9ba18c857
Part 1: Support passing name length to JS_GetOwnUCPropertyDescriptor and add JS_GetUCPropertyDescriptor; r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/40225b349c7c
Part 2: Avoid searching the prototype chain when extracting keys; r=asuth

Comment 8

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #6)
> ::: dom/indexedDB/KeyPath.cpp
> @@ +166,5 @@
> > +                  JS_NewUCStringCopyN(aCx, name.get(), name.Length());
> > +
> > +                intermediate = JS::StringValue(string);
> > +                hasProp = true;
> > +              } else if (token.EqualsLiteral("lastModified")) {
> 
> It might be worth calling out in a comment that although the spec also lists
> "lastModifiedDate" that we deprecated and removed support for it.

Ok, added a comment.

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/96c9ba18c857
https://hg.mozilla.org/mozilla-central/rev/40225b349c7c

Comment 10

[Pascal Chevrel:pascalc]

Jan, should that patch be uplifted to Beta?

Comment 11

[Jan Varga [:janv]]

Comment on attachment 9010624 [details] [diff] [review]
Part 2: Avoid searching the prototype chain when extracting keys

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Longstanding IndexedDB correctness issue.

User impact if declined: Web developers may be more inclined to fall back to LocalStorage which is all around a worse API for developers and users, or give Firefox a degraded experience.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): We have thorough automated tests.

String changes made/needed: No string changes.

Comment 12

[Pascal Chevrel:pascalc]

Comment on attachment 9010624 [details] [diff] [review]
Part 2: Avoid searching the prototype chain when extracting keys

Low risk patch that has been on nightly for 2 weeks, approved for the last 63 beta, thanks.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

FYI, the patch grafts cleanly to ESR60 as well, but hits test failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=204790357&repo=try&lineNumber=2179
https://treeherder.mozilla.org/logviewer.html#?job_id=204791396&repo=try&lineNumber=7101

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d43c7f7c0d15
https://hg.mozilla.org/releases/mozilla-beta/rev/1c577d6a48f0

Comment 15

[Jan Varga [:janv]]

Attachment: Part 1 for ESR60

Comment 16

[Jan Varga [:janv]]

Attachment: Part 2 for ESR60

Comment 17

[Jan Varga [:janv]]

Comment on attachment 9017265 [details] [diff] [review]
Part 1 for ESR60

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Longstanding IndexedDB correctness issue and also see bug 1487660.

User impact if declined: Web developers may be more inclined to fall back to LocalStorage which is all around a worse API for developers and users, or give Firefox a degraded experience.

Fix Landed on Version: 64 and 63

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): We have thorough automated tests.

String or UUID changes made by this patch: No string changes.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9017265 [details] [diff] [review]
Part 1 for ESR60

Thanks for doing the rebase. I've confirmed that the wpt failures I saw on my Try push are gone now. Approved for 60.3esr.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/eeb9060379dc
https://hg.mozilla.org/releases/mozilla-esr60/rev/99eae0d15092