Closed Bug 1309527 Opened 3 years ago Closed 3 years ago

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

Categories

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

49 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: jgraham, Assigned: bevis)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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
Bevis, can you investigate?
Flags: needinfo?(btseng)
I'll look into it after these disabled wpts are imported.
Assignee: nobody → btseng
Flags: needinfo?(btseng)
Priority: -- → P3
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
Blocks: 1118028
Keywords: regression
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
Attachment #8804627 - Flags: review?(jvarga)
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!
Attachment #8804627 - Flags: review?(jvarga) → review+
Rebase to remove idbindex-rename.html.ini and idbobjectstore-rename-store.html.ini from wpt metadata.
Attachment #8804627 - Attachment is obsolete: true
Attachment #8806210 - Flags: review+
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
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
Flags: needinfo?(btseng)
Pushed by archaeopteryx@coole-files.de:
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
Relanded because the issue seemed to be caused by bug 1314037.
Flags: needinfo?(btseng)
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.
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.
(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.
* 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
Attachment #8806210 - Attachment is obsolete: true
Attachment #8807517 - Flags: review+
checkin-needed again after fixing the timeout problem in comment 15.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f98d9da756c7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Version: unspecified → 49 Branch
You need to log in before you can comment on or make changes to this bug.