1309527 - Random crashes in wpt tests /IndexedDB/idbobjectstore-rename-store.html and /IndexedDB/idbindex-rename.html

Status: RESOLVED FIXED

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

Description

[James Graham [:jgraham]]

Trying to do a wpt update in [1] I see random asserts/crashes in those tests in debug builds.

I think I will have to disable the tests for now at least on debug platforms.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=cde0dffd046d&selectedJob=29025085

Comment 1

[Andrew Overholt [:overholt]]

Bevis, can you investigate?

Comment 2

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

I'll look into it after these disabled wpts are imported.

Comment 3

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

Attachment: (v1) Patch: Fix the race condition to prevent the access of metadata in both PBackground thread and the Connection thread.

Root cause was found.
There is a race condition in the fix of bug 1118028.
The metadata could be updated in PBackgroundThread[1] and read in the connection thread[2].

This cause the assertion when swapping the indexes/objectores:
>        const store = transaction.objectStore('books');
>        store.index('by_author').name = 'tmp';
>        store.index('by_title').name = 'by_author';
>        store.index('tmp').name = 'by_title';

We should copy the id & name for the rename operation instead of accessing the same metadata that could be modified while rename operation is ongoing.

Wait for the treeherder result complete to start the review:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6015d32ebd7462432cae886fa64a647d0afa889

[1] http://searchfox.org/mozilla-central/source/dom/indexedDB/ActorsParent.cpp#15798
[2] http://searchfox.org/mozilla-central/source/dom/indexedDB/ActorsParent.cpp#23819

Comment 4

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

Comment on attachment 8804627 [details] [diff] [review]
(v1) Patch: Fix the race condition to prevent the access of metadata in both PBackground thread and the Connection thread.

The root cause is a race condition while supporting the renaming of IDBIndex and IDBObjectStore in bug 1118028:
The metadata could be updated in PBackgroundThread[1] and be read in the connection thread [2] for the rename operation.

This hits the assertion when swapping the indexes/objectores in these wpt test case:
>        const store = transaction.objectStore('books');
>        store.index('by_author').name = 'tmp';
>        store.index('by_title').name = 'by_author';
>        store.index('tmp').name = 'by_title';

We should copy the id & name for the rename operation instead of accessing the same metadata that could be modified while rename operation is ongoing. :|

The wpt result in treeherder looks fine now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6015d32ebd7

Hi Jan,

May I have your review for this fix?

Thanks!

[1] http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/dom/indexedDB/ActorsParent.cpp#16130
[2] http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/dom/indexedDB/ActorsParent.cpp#25510,25541

Comment 5

[Jan Varga [:janv]]

Comment on attachment 8804627 [details] [diff] [review]
(v1) Patch: Fix the race condition to prevent the access of metadata in both PBackground thread and the Connection thread.

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

Looks good. Thanks!

Comment 6

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

Attachment: (final) Patch: Fix the race condition to prevent the access of metadata in both PBackground thread and the Connection thread. r=janv

Rebase to remove idbindex-rename.html.ini and idbobjectstore-rename-store.html.ini from wpt metadata.

Comment 7

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

treeherder result looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=450e4d324442

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bbfcf5356f7
Fix the race condition to prevent the access of metadata in both PBackground thread and the Connection thread. r=janv

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for timing out in idbobjectstore-rename-store.html on Windows x64 opt:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1633ac89c6ac66367461e9f93dab995bfcf068b4

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=38559683&repo=mozilla-inbound

09:34:45     INFO - TEST-START | /IndexedDB/idbobjectstore-rename-store.html
09:34:55     INFO - PROCESS | 2252 | MARIONETTE LOG: INFO: Timeout fired
09:34:55     INFO - 
09:34:55     INFO - TEST-PASS | /IndexedDB/idbobjectstore-rename-store.html | IndexedDB object store rename in new transaction 
09:34:55     INFO - TEST-PASS | /IndexedDB/idbobjectstore-rename-store.html | IndexedDB object store rename in the transaction where it is created 
09:34:55     INFO - TEST-PASS | /IndexedDB/idbobjectstore-rename-store.html | IndexedDB object store rename covers index 
09:34:55     INFO - TEST-PASS | /IndexedDB/idbobjectstore-rename-store.html | IndexedDB object store rename covers key generator 
09:34:55     INFO - TEST-PASS | /IndexedDB/idbobjectstore-rename-store.html | IndexedDB object store rename to the same name succeeds 
09:34:55     INFO - TEST-PASS | /IndexedDB/idbobjectstore-rename-store.html | IndexedDB object store rename to the name of a deleted store succeeds 
09:34:55     INFO - TEST-PASS | /IndexedDB/idbobjectstore-rename-store.html | IndexedDB object store swapping via renames succeeds 
09:34:55     INFO - TEST-PASS | /IndexedDB/idbobjectstore-rename-store.html | IndexedDB object store rename stringifies non-string names 
09:34:55     INFO - TEST-PASS | /IndexedDB/idbobjectstore-rename-store.html | IndexedDB object store can be renamed to "" 
09:34:55     INFO - TEST-UNEXPECTED-TIMEOUT | /IndexedDB/idbobjectstore-rename-store.html | IndexedDB object store can be renamed to "\u0000" - Test timed out
09:34:55     INFO - 
09:34:55     INFO - TEST-UNEXPECTED-NOTRUN | /IndexedDB/idbobjectstore-rename-store.html | IndexedDB object store can be renamed to "\uDC00\uD800" - expected PASS
09:34:55     INFO - TEST-UNEXPECTED-TIMEOUT | /IndexedDB/idbobjectstore-rename-store.html | expected OK

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e8791563f4d
Fix the race condition to prevent the access of metadata in both PBackground thread and the Connection thread. r=janv

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Relanded because the issue seemed to be caused by bug 1314037.

Comment 12

[Phil Ringnalda (:philor)]

Oh, you put the patch in and you take the patch you, you put the patch in and you take the patch out...


Backed out yet again in https://hg.mozilla.org/integration/mozilla-inbound/rev/f8334efe84730204eab8e6f4411d75990232f4fc, because https://treeherder.mozilla.org/logviewer.html#?job_id=38588733&repo=mozilla-inbound - apparently the problem is either one of them.

Comment 13

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

Ii didn't catch this problem in treeherder because the tests were not run by default in windows 8. :(
It is different issue from what to be fixed in this bug.
I'll disable that single test item for windows 8 first and file another bug to follow up.

Comment 14

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

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #13)
> Ii didn't catch this problem in treeherder because the tests were not run by
> default in windows 8. :(
> It is different issue from what to be fixed in this bug.
> I'll disable that single test item for windows 8 first and file another bug
> to follow up.

The timeout looks expected in windows 8 before landing this patch:
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/testing/web-platform/meta/IndexedDB/idbobjectstore-rename-store.html.ini#9,12,16,20-21

Instead of removing these .ini files, I should just remove the expected CRASH from these .ini and keep the expected TIMEOUT/NORETURN.

I'll do more testing in treeherder to ensure that these errors will be identified in the .ini file and file a follow-up bug for this.

Comment 15

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

Attachment: (v3) Patch: Fix the race condition to prevent the access of metadata in both PBackground thread and the Connection thread. r=janv

* Extend the timeout for these 2 test cases.

The reason of these TIMEOUT is that in these 2 test cases, there are about 10 database opening and renaming in each test case, and the testing time in average is 11 seconds in windows 8 and is 8 seconds in windows 7.
That's why we see these TIMEOUT quite frequent in windows 8.
I also saw TIMEOUT happens 2 times in windows 7 while testing.

After applying this new patch to extend the timeout, the TIMEOUT problem is gone according to the treeherder result(40 runs in each platform):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e0af9f4b580&group_state=expanded

Comment 16

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

checkin-needed again after fixing the timeout problem in comment 15.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f98d9da756c7
Fix the race condition to prevent the access of metadata in both PBackground thread and the Connection thread. r=janv

Comment 18

[Phil Ringnalda (:philor)]

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