Summary: | Layout Test storage/indexeddb/intversion-omit-parameter.html is flaky | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Pavlov (apavlov) <apavlov> | ||||||||||
Component: | Tools / Tests | Assignee: | Joshua Bell <jsbell> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | alecflett, dgrogan, jsbell, pkasting, tony, webkit.review.bot, xingnan.wang | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Alexander Pavlov (apavlov)
2012-08-01 23:52:58 PDT
This test was recently added, in r124383. Perhaps the test is inherently flaky? Hopefully one of the CCed folks has the expertise to look into this. Xingnan, don't worry about this, it's not related to your patch. (In reply to comment #2) > Xingnan, don't worry about this, it's not related to your patch. That`s fine. BTW, I cannot reproduce it on my ubuntu11.10. I can get content_shell (without any upgradeneeded code) to fail an assert on a good portion of IDB layout tests by hitting refresh a couple times (IDB's constant nemesis) or even just back <-> forward. The pattern is * There's a pending call on the backend, either open/close/deleteDatabase or just an openCursor that hasn't been processed * ActiveDOMObjects get stop()'ped, IDBRequest gets marked EarlyDeath * Backend proceeds with pending call * IDBRequest::on{Success,Error} is called with the results of the pending call * ASSERT(m_readyState == PENDING || m_readyState == DONE) fails because IDBRequest.m_readyState == EarlyDeath Josh, do you remember off the top of your head if we had a mechanism in place to prevent this flow? (In reply to comment #4) > I can get content_shell (without any upgradeneeded code) to fail an assert on a good portion of IDB layout tests by hitting refresh a couple times (IDB's constant nemesis) or even just back <-> forward. > > The pattern is > * There's a pending call on the backend, either open/close/deleteDatabase or just an openCursor that hasn't been processed > * ActiveDOMObjects get stop()'ped, IDBRequest gets marked EarlyDeath > * Backend proceeds with pending call > * IDBRequest::on{Success,Error} is called with the results of the pending call > * ASSERT(m_readyState == PENDING || m_readyState == DONE) fails because IDBRequest.m_readyState == EarlyDeath > > Josh, do you remember off the top of your head if we had a mechanism in place to prevent this flow? Not specifically. That assert is new, though - I didn't consider the stopped case. :( The asserts in any of the onXXX callbacks can probably be removed (inspect that I didn't over-simplify the code though). The enque/dispatch code handles the stopped case. I'll tackle this on Monday. Created attachment 156708 [details]
Patch
Created attachment 156711 [details]
Patch
A direct layout test for this might be possible: initiate IDB operations (like in the omit test) then close the document before the pending events can be processed. Possibly in an iframe, so the top-level page persists and can see the eventual success events. I'll play with it. In the mean time, dgrogan@ can you take a look and try the patch on your system to see if it does resolve the content shell repro you had? Created attachment 156975 [details]
Patch
(In reply to comment #8) > A direct layout test for this might be possible: initiate IDB operations (like in the omit test) then close the document before the pending events can be processed. Possibly in an iframe, so the top-level page persists and can see the eventual success events. I gave up on the layout test and went with a Chromium unit test instead (in latest patch). alecflett@, dgrogan@ - please take a look? Most of the IDBRequest::onXXX() callbacks now have the same preamble. I considered extracting a private method that would encapsulate the "if stopped or aborted then bail early" logic and state assertions, e.g. "if (!shouldProcessEvent()) return;" - thoughts? +1 for consolidation I seem to have just started hitting these ASSERTs (that are being removed here) in my createIndex patch. (way more in the browser tests than in DRT) I was in the process of trying to break it up when I started hitting them and this will certainly keep me from hitting them - is this happening more with the recent patches from david? Mostly I'm just trying to figure out if this what I'm running into, or if I'm inadvertently introducing bugs as I break up my mega patch :( (In reply to comment #11) > I seem to have just started hitting these ASSERTs (that are being removed here) in my createIndex patch. (way more in the browser tests than in DRT) I was in the process of trying to break it up when I started hitting them and this will certainly keep me from hitting them - is this happening more with the recent patches from david? Mostly I'm just trying to figure out if this what I'm running into, or if I'm inadvertently introducing bugs as I break up my mega patch :( Yeah, dgrogan@'s patch makes the pending calls queue around database open/version/close/delete more necessarily more complex, and introduces more opportunities for events to get triggered after context destruction which is the trigger for these asserts. I'll re-up the patch with common logic factored out. Created attachment 157003 [details]
Patch
tony@ - r? LGTM, thanks for fixing this. Comment on attachment 157003 [details] Patch Clearing flags on attachment: 157003 Committed r124974: <http://trac.webkit.org/changeset/124974> All reviewed patches have been landed. Closing bug. |