IDBKeyRange.includes() works improperly if only the lower bound is set.

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bevis, Assigned: bevis)

Tracking

({regression})

47 Branch
mozilla50
regression
Points:
---

Firefox Tracking Flags

(firefox47 wontfix, firefox48 wontfix, firefox49- fixed, firefox50 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
The specified key is unexpectedly compared with the *unset* upper bound when the key is identical or larger then the lower bound.

Note: This is reported by Chrome Engineer with enhanced web-platform test included:
http://stackoverflow.com/questions/37780602/confusing-behavior-from-idbkeyrange-includes/37949910#37949910
(Assignee)

Comment 1

2 years ago
Created attachment 8764155 [details] [diff] [review]
(v1) Patch: Do Not Compare to The Unset Boundary.

1. Make it clear that at least one boundary shall be set in a valid IDBKeyRange instace.
2. Do the comparison only if the boundary is set.
3. Copy the test case being reviewed in https://github.com/w3c/web-platform-tests/pull/3216 for uplifting to release/beta/aurora channel if needed. (The test case in this patch is not required for mozilla-central once it is merged from github.com/w3c/web-platform-tests.
Attachment #8764155 - Flags: review?(khuey)
(Assignee)

Updated

2 years ago
Whiteboard: btpp-active
Comment on attachment 8764155 [details] [diff] [review]
(v1) Patch: Do Not Compare to The Unset Boundary.

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

Your boolean logic needs some work here.

Please name this "In IDBKeyRange.includes(), do not ..." or something similar.  Also follow normal English capitalization rules in a sentence ;)

::: dom/indexedDB/IDBKeyRange.cpp
@@ +348,5 @@
>  
> +  MOZ_ASSERT(!(Lower().IsUnset() && Upper().IsUnset()));
> +  MOZ_ASSERT_IF(IsOnly(),
> +    Lower() == Upper() && LowerOpen() == UpperOpen() &&
> +    !Lower().IsUnset() && !LowerOpen());

It would make a little more sense to assert these in the other order. i.e.

!Lower().IsUnset() && !LowerOpen() && Lower() == Upper() && LowerOpen() == UpperOpen()

Verify that the lower values are what you expect, and then verify that the upper values match.

@@ +368,2 @@
>      }
> +    MOZ_ASSERT_IF(IsOnly(), key == Lower());

Huh?  The only way we can even get to this point is if we got case -1 and !IsOnly().  So this assertion is useless.

@@ +370,3 @@
>    }
>  
> +  if (!IsOnly() && !Upper().IsUnset()) {

How do we get here if IsOnly()?  You asserted above that if IsOnly() then !Lower().IsUnset(), and we established above that we only exit the switch if !IsOnly().
Attachment #8764155 - Flags: review?(khuey) → review-
(Assignee)

Comment 3

2 years ago
Comment on attachment 8764155 [details] [diff] [review]
(v1) Patch: Do Not Compare to The Unset Boundary.

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

::: dom/indexedDB/IDBKeyRange.cpp
@@ +348,5 @@
>  
> +  MOZ_ASSERT(!(Lower().IsUnset() && Upper().IsUnset()));
> +  MOZ_ASSERT_IF(IsOnly(),
> +    Lower() == Upper() && LowerOpen() == UpperOpen() &&
> +    !Lower().IsUnset() && !LowerOpen());

That's my 1st version of this assertion, too.
I revised this because I wonder if the equality shall be prior to the value of the lower bound. :p
I'll roll back the change since it makes more sense to us.

@@ +368,2 @@
>      }
> +    MOZ_ASSERT_IF(IsOnly(), key == Lower());

My bad!!
I didn't remove this assertion after changing the line to return !LowerOpen() if the value is identical.

@@ +370,3 @@
>    }
>  
> +  if (!IsOnly() && !Upper().IsUnset()) {

ditto. :-(
(Assignee)

Comment 4

2 years ago
Created attachment 8764520 [details] [diff] [review]
(v2) Patch: In IDBKeyRange.includes(), do not compare the given value with the unset boundary.

address the issues in comment 2.
Attachment #8764155 - Attachment is obsolete: true
Attachment #8764520 - Flags: review?(khuey)
(Assignee)

Comment 5

2 years ago
treeherder result for reference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fcfd04dcf9f

Comment 6

2 years ago
This is not something we'd like to include in a dot release.
status-firefox47: affected → wontfix
Attachment #8764520 - Flags: review?(khuey) → review+
(Assignee)

Comment 7

2 years ago
Comment on attachment 8764520 [details] [diff] [review]
(v2) Patch: In IDBKeyRange.includes(), do not compare the given value with the unset boundary.

This patch fixes the failure of the upcoming change in [1].

The change of idbkeyrange-includes.htm in this patch is the same copy of the change in [1].

Set NI to see how to fix this bug without conflict to the periodic merge of w3c-web-platform-tests:

Should I just land this patch or wait until an expected failure specified in idbkeyrange-includes.htm.ini after periodic merge from w3c-webplatform-tests?

[1] https://github.com/w3c/web-platform-tests/commit/fed03d61ac548fb94838e0f868f75c619d9a8edb
Flags: needinfo?(james)
James will handle the merge, which should be trivial, if both us and upstream update to the exact same file.
(Assignee)

Comment 9

2 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8)
> James will handle the merge, which should be trivial, if both us and
> upstream update to the exact same file.

I see! Cancel the NI and ask for checkin-needed. (y)

Treeherder result is in comment 5, BTW.
Flags: needinfo?(james)
Keywords: checkin-needed

Comment 10

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b73d5f7b9f
In IDBKeyRange.includes(), do not compare the given value with the unset boundary. r=khuey
Keywords: checkin-needed

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f0b73d5f7b9f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Should we consider uplifting this?
Flags: needinfo?(btseng)
Version: unspecified → 47 Branch
(Assignee)

Comment 13

a year ago
[Tracking Requested - why for this release]:

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Should we consider uplifting this?

I thought that the decision will be made after regression is identified and status flags were reviewed.
Or maybe I should ask for the tracking flags instead.
I'll set the approval flags later once the treeherder result in aurora/beta branches are done.

Keep NI on me.

[Tracking Requested - why for this release]:
We got compliant from web developers that the API of IDBKeyRange.includes() works improperly if only the lower bound is set in our first release of this API in bug 1251498. [1]

In this bug, we have fixed the problem to avoid unexpected comparison when part of the boundary is not set and updated the web-platform test to improve our test coverage of the boundary check.

[1] http://stackoverflow.com/questions/37780602/confusing-behavior-from-idbkeyrange-includes/37949910#37949910
tracking-firefox47: --- → ?
tracking-firefox48: --- → ?
tracking-firefox49: --- → ?
(Assignee)

Comment 14

a year ago
Clear tracking-firefox47 since status-firefox47 as been marked as wontfix.
tracking-firefox47: ? → ---
If we've already shipped the wrong behavior I don't think uplifting this is very important.
(Assignee)

Comment 16

a year ago
Comment on attachment 8764520 [details] [diff] [review]
(v2) Patch: In IDBKeyRange.includes(), do not compare the given value with the unset boundary.

Approval Request Comment
[Feature/regressing bug #]:Regression of bug 1251498 which didn't properly take care of the case when only lower bound is set.
[User impact if declined]:Annoying to web developer/user because unexpected result is returned:
http://stackoverflow.com/questions/37780602/confusing-behavior-from-idbkeyrange-includes/37949910#37949910
[Describe test coverage new/current, TreeHerder]:Updated web-platform test(idbkeyrange-includes.htm) is included to improve the test coverage.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=97f8ff29b6c5&selectedJob=23632501
[Risks and why]: Low. The fix makes this API works properly without interfering other parts.
[String/UUID change made/needed]:none
Flags: needinfo?(btseng)
Attachment #8764520 - Flags: approval-mozilla-beta?
Attachment #8764520 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 17

a year ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #15)
> If we've already shipped the wrong behavior I don't think uplifting this is
> very important.

There is no much effort to me to have this uplifted because the same patch could be uplifted to aurora/beta without conflict and the risk is low.
Hence, I left these requests here for the release manager to pick it up if needed. :)
Hi Bevis,
If we let this ride the train on 49/50 only and won't fix in 48, will this be a big impact for user for 48?
Flags: needinfo?(btseng)
(Assignee)

Comment 19

a year ago
(In reply to Gerry Chang [:gchang] from comment #18)
> Hi Bevis,
> If we let this ride the train on 49/50 only and won't fix in 48, will this
> be a big impact for user for 48?

No, it won't because this is already an known issue in 47 and we only got 1 compliant from StackOverFlow so far.
Flags: needinfo?(btseng)
Comment on attachment 8764520 [details] [diff] [review]
(v2) Patch: In IDBKeyRange.includes(), do not compare the given value with the unset boundary.

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

Per comment #19, no huge user impact for this.
Attachment #8764520 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Per comment #20, un-track and won't fix for 48.
status-firefox48: affected → wontfix
tracking-firefox48: ? → ---
Comment on attachment 8764520 [details] [diff] [review]
(v2) Patch: In IDBKeyRange.includes(), do not compare the given value with the unset boundary.

This patch fixes unset boundary in IDBKeyRange.includes(). Take it in 49 aurora.
Attachment #8764520 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Un-track 49 as there is no huge user impact and we only got 1 compliant from StackOverFlow so far.
tracking-firefox49: ? → -

Comment 24

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/d1475df3d729
status-firefox49: affected → fixed
You need to log in before you can comment on or make changes to this bug.