Bug 72037 – storage/indexeddb/factory-deletedatabase-interactions.html is very flaky

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Ojan Vafai]

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=storage%2Findexeddb%2Ffactory-deletedatabase-interactions.html&showAllRuns=true

Looks like it was flaky before the changes arv made to the test. So, either something in WebCore got flaky or the test has always been flaky.

Seems like the 4th test in that file is sometimes failing by only having one close call instead of two.

Given that the test is expected to fail (just in another way), I'd propose to disable the 4th test in that file entirely until the bug (not sending version change events at closing connections) is fixed and the text should pass

wdyt

Created attachment 114539 [details]
Patch

[Ojan Vafai]

Comment on attachment 114539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114539&action=review

> LayoutTests/storage/indexeddb/factory-deletedatabase-interactions.html:190
> +        // Disabling test4 due to flakiness in the way that test doesn't work.

This should also include a link to the bug for fixing the root problem so that eventually someone can see the bug was fixed and renable this.

Created attachment 114548 [details]
Patch

[Ojan Vafai]

Comment on attachment 114548 [details]
Patch

Thanks for fixing this!

[Joshua Bell]

Comment on attachment 114548 [details]
Patch

The flakyness is because the second deleteDatabase()'s onsuccess event may be delivered before or after the setTimeout-delayed close() call; it's not an IDB issue, so I don't think we'd ever be able to rely on the exact ordering. One way to fix would be to only call h.close() once (e.g. have a flag that is checked/set)

(In reply to comment #6)
> (From update of attachment 114548 [details])
> The flakyness is because the second deleteDatabase()'s onsuccess event may be delivered before or after the setTimeout-delayed close() call; it's not an IDB issue, so I don't think we'd ever be able to rely on the exact ordering. One way to fix would be to only call h.close() once (e.g. have a flag that is checked/set)

ok, I see your point.

Given that test4 is supposed to test not-yet-existing behavior (i.e. how the closePending flag affects deleteDatabase), maybe instead of disabling test4, I should just remove test4?

Comment on attachment 114548 [details]
Patch

removing the test instead of disabling it

Created attachment 114563 [details]
Patch

[Ojan Vafai]

Comment on attachment 114563 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114563&action=review

> LayoutTests/ChangeLog:7
> +        The test isn't working, but it's failing in different ways. Disabling
> +        it, until the underlying issue is fixed.

Would be nice to update the description to match the new code, e.g. "Removing the test since it's testing not-yet-written code. Once the new code is written it will obviously come with the appropriate tests."

Created attachment 114567 [details]
Patch

Comment on attachment 114567 [details]
Patch

Clearing flags on attachment: 114567

Committed r100007: <http://trac.webkit.org/changeset/100007>

All reviewed patches have been landed.  Closing bug.