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

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (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.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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().

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

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. :-(

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Patch: In IDBKeyRange.includes(), do not compare the given value with the unset boundary.

address the issues in comment 2.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 6

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This is not something we'd like to include in a dot release.

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

James will handle the merge, which should be trivial, if both us and upstream update to the exact same file.

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b73d5f7b9f
In IDBKeyRange.includes(), do not compare the given value with the unset boundary. r=khuey

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f0b73d5f7b9f

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Should we consider uplifting this?

Comment 13

[Bevis Tseng[:bevis][:btseng](Exited)]

[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

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Clear tracking-firefox47 since status-firefox47 as been marked as wontfix.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

If we've already shipped the wrong behavior I don't think uplifting this is very important.

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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. :)

Comment 18

[Gerry Chang [:gchang]]

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?

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

(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.

Comment 20

[Gerry Chang [:gchang]]

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.

Comment 21

[Gerry Chang [:gchang]]

Per comment #20, un-track and won't fix for 48.

Comment 22

[Gerry Chang [:gchang]]

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.

Comment 23

[Gerry Chang [:gchang]]

Un-track 49 as there is no huge user impact and we only got 1 compliant from StackOverFlow so far.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d1475df3d729