IndexedDB: Fix null ptr crash in IDBCursor::value().

IndexedDB: Fix null ptr crash in IDBCursor::value().

A better approach than the initial fix that landed in #350774.
This one should catch a broader rage of situations in which the
cursor's value is read before being made ready.

BUG=535119
Committed: https://crrev.com/28b60de585cb8ea61421d379ad18bf3f0dc057b9
Cr-Commit-Position: refs/heads/master@{#351680}

[cmumford, 2015-09-25 17:41:26]

cmumford@chromium.org changed reviewers:
+ jsbell@chromium.org

[cmumford, 2015-09-25 17:41:27]

jsbell:

I like this solution better than my original fix in
https://codereview.chromium.org/1360163003. If you +1 then I'll add a reviewer
for V8BindingForModules.cpp

[jsbell, 2015-09-28 17:11:29]

https://codereview.chromium.org/1369773004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/1369773004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp:324: if (!m_value) {
From code inspection, it looks like this can happen at the point where a cursor
has been run to the end of its range:

IDBRequest::onSuccess(PassRefPtr<IDBValue>) where the value is null and
m_pendingCursor is true. This calls IDBCursor::close().

Can we add a layout test?

[cmumford, 2015-09-28 17:12:37]

On 2015/09/28 17:11:29, jsbell wrote:
>
https://codereview.chromium.org/1369773004/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp (right):
> 
>
https://codereview.chromium.org/1369773004/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp:324: if (!m_value) {
> From code inspection, it looks like this can happen at the point where a
cursor
> has been run to the end of its range:
> 
> IDBRequest::onSuccess(PassRefPtr<IDBValue>) where the value is null and
> m_pendingCursor is true. This calls IDBCursor::close().
> 
> Can we add a layout test?

Sure.

[cmumford, 2015-09-30 18:49:40]

Test passes in FF too.

[jsbell, 2015-09-30 20:34:06]

https://chromiumcodereview.appspot.com/1369773004/diff/20001/third_party/WebK...
File
third_party/WebKit/LayoutTests/storage/indexeddb/cursor-after-range-bug.html
(right):

https://chromiumcodereview.appspot.com/1369773004/diff/20001/third_party/WebK...
third_party/WebKit/LayoutTests/storage/indexeddb/cursor-after-range-bug.html:7:
function doSetup(dbName, dbVersion, onsuccess) {
You should make this all part of the async_test, otherwise the asserts don't
mean anything.

(Also, I think we have a testharness helper function for IDB tests, don't we?)

https://chromiumcodereview.appspot.com/1369773004/diff/20001/third_party/WebK...
third_party/WebKit/LayoutTests/storage/indexeddb/cursor-after-range-bug.html:35:
req.onsuccess = function(evt) {
Callbacks need to be wrapped in t.step_func, e.g.

req.onsuccess = t.step_func(function(evt) { ... });

... so that an assertion failure causes test t to fail.

https://chromiumcodereview.appspot.com/1369773004/diff/20001/third_party/WebK...
third_party/WebKit/LayoutTests/storage/indexeddb/cursor-after-range-bug.html:41:
assert_equals(last_cursor.value, undefined);
Cool - that does match the spec. :) I wasn't sure.

https://chromiumcodereview.appspot.com/1369773004/diff/20001/third_party/WebK...
third_party/WebKit/LayoutTests/storage/indexeddb/cursor-after-range-bug.html:45:
req.onerror = function() {
ditto

[jsbell, 2015-09-30 20:34:22]

On 2015/09/30 18:49:40, cmumford wrote:
> Test passes in FF too.

Awesome, thanks for checking.

[jsbell, 2015-09-30 20:51:47]

Also, c++ side and test semantics l*g*t*m but I'll take another look at the test
when it's ready.

indexeddb_test() in resources/testharness-helpers.js is what I was thinking of.

[cmumford, 2015-09-30 21:59:44]

https://codereview.chromium.org/1369773004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/storage/indexeddb/cursor-after-range-bug.html
(right):

https://codereview.chromium.org/1369773004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/cursor-after-range-bug.html:7:
function doSetup(dbName, dbVersion, onsuccess) {
On 2015/09/30 20:34:06, jsbell wrote:
> You should make this all part of the async_test, otherwise the asserts don't
> mean anything.
> 
> (Also, I think we have a testharness helper function for IDB tests, don't we?)

Done.

https://codereview.chromium.org/1369773004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/cursor-after-range-bug.html:35:
req.onsuccess = function(evt) {
On 2015/09/30 20:34:06, jsbell wrote:
> Callbacks need to be wrapped in t.step_func, e.g.
> 
> req.onsuccess = t.step_func(function(evt) { ... });
> 
> ... so that an assertion failure causes test t to fail.

Done.

https://codereview.chromium.org/1369773004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/cursor-after-range-bug.html:41:
assert_equals(last_cursor.value, undefined);
On 2015/09/30 20:34:06, jsbell wrote:
> Cool - that does match the spec. :) I wasn't sure.

Acknowledged.

https://codereview.chromium.org/1369773004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/cursor-after-range-bug.html:45:
req.onerror = function() {
On 2015/09/30 20:34:06, jsbell wrote:
> ditto

Done.

[jsbell, 2015-09-30 22:23:01]

lgtm

[jsbell, 2015-09-30 22:23:02]

lgtm

[jsbell, 2015-09-30 22:23:07]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2015-09-30 22:26:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1369773004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1369773004/40001

[commit-bot: I haz the power, 2015-09-30 22:37:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-30 22:37:12]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jsbell, 2015-09-30 22:42:24]

On 2015/09/30 22:37:12, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Ooops, yeah, bindings.

Honestly, you could leave that code as-is though.

[cmumford, 2015-09-30 23:03:11]

The CQ bit was checked by cmumford@chromium.org

[cmumford, 2015-09-30 23:03:11]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1369773004/#ps60001
(title: "Reverted null ptr check removal")

[commit-bot: I haz the power, 2015-09-30 23:04:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1369773004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1369773004/60001

[commit-bot: I haz the power, 2015-10-01 00:15:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-01 00:16:31]

Patchset 4 (id:??) landed as
https://crrev.com/28b60de585cb8ea61421d379ad18bf3f0dc057b9
Cr-Commit-Position: refs/heads/master@{#351680}