Closed Bug 1492737 Opened Last year Closed Last year

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 --- fixed
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: janv, Assigned: janv)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

No description provided.
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Priority: -- → P2
Blocks: 1178829
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.
Attachment #9010623 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #9010624 - Flags: review?(bugmail) → review+
Pushed by jvarga@mozilla.com:
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
(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.
https://hg.mozilla.org/mozilla-central/rev/96c9ba18c857
https://hg.mozilla.org/mozilla-central/rev/40225b349c7c
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Jan, should that patch be uplifted to Beta?
Flags: needinfo?(jvarga)
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.
Flags: needinfo?(jvarga)
Attachment #9010624 - Flags: approval-mozilla-beta?
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.
Attachment #9010624 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached patch Part 1 for ESR60Splinter Review
Attached patch Part 2 for ESR60Splinter Review
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.
Attachment #9017265 - Flags: approval-mozilla-esr60?
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.
Attachment #9017265 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.