Bug 152397

Created attachment 267583 [details] Patch v1

This appears to have made multiple tests crash under GuardMalloc. Will roll out. CRASHING TEST: storage/indexeddb/delete-in-upgradeneeded-close-in-open-success.html This process is running with libgmalloc.dylib (GuardMalloc) which may have forced the crash due to a memory access error. Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x000000010ba998f3 WebCore::IDBServer::UniqueIDBDatabase::operationAndTransactionTimerFired() + 35 1 com.apple.WebCore 0x000000010aa84c30 WebCore::ThreadTimers::sharedTimerFiredInternal() + 176 2 com.apple.WebCore 0x000000010aa84b6f WebCore::timerFired(__CFRunLoopTimer*, void*) + 31 3 com.apple.CoreFoundation 0x00007fff87a6a414 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20

Re-opened since this is blocked by bug 152415

(In reply to comment #3) > This appears to have made multiple tests crash under GuardMalloc. Will roll > out. Since standard practice for individual patch authors has never including running tests under GuardMalloc, and since we don't have GuardMalloc EWS, and since GuardMalloc bots aren't even on the bot watcher's dashboard, I object to the policy of rolling out without giving patch authors a reasonable amount of time to take a look.

(In reply to comment #5) > (In reply to comment #3) > > This appears to have made multiple tests crash under GuardMalloc. Will roll > > out. > > Since standard practice for individual patch authors has never including > running tests under GuardMalloc, and since we don't have GuardMalloc EWS, > and since GuardMalloc bots aren't even on the bot watcher's dashboard, I > object to the policy of rolling out without giving patch authors a > reasonable amount of time to take a look. Especially since this also required the compound rollout of a followup patch. (https://bugs.webkit.org/show_bug.cgi?id=152405) *sigh*

Created attachment 267610 [details] Patch for landing

(In reply to comment #7) > Created attachment 267610 [details] > Patch for landing For posterity: The GuardMalloc failure was your typical use-after-free, in this case the UniqueIDBDatabase doing something to cause itself to get deleted while in the middle of handling a timer callback. There's a few ways to have solved this, and I went with our typical RefPtr<> protector(this) pattern inside the timer callback. This can be revisited at any future date.

> Since standard practice for individual patch authors has never including running tests under GuardMalloc Rolling out is not a punishment, it has nothing to do with how hard or was it was to avoid the problem. This is something that is done for the benefit of others, as flakiness costs everyone a lot of time - from EWS being slower to analyzing induced memory corruption crashes to fluctuations on performance metrics. As we add more styles of testing, I expect that we will be only getting more quick to roll back.

Debug EWS is still orange - there is a streams crash, and IndexedDB flakiness too.

Comment on attachment 267610 [details] Patch for landing Clearing flags on attachment: 267610 Committed r194263: <http://trac.webkit.org/changeset/194263>

All reviewed patches have been landed. Closing bug.

(In reply to comment #9) > > Since standard practice for individual patch authors has never including running tests under GuardMalloc > > Rolling out is not a punishment, it has nothing to do with how hard or was > it was to avoid the problem. I didn't claim it was a punishment. Last night I could've either: 1 - Made a quick fix for the GuardMalloc issues and then get 1 or 2 more bug fixes done 2 - Update past the rollout, build, get a new patch ready, submit it, then have no time to work on addition bug fixes as planned. I did #2, meaning new bug fixes were not done last night. > > This is something that is done for the benefit of others, as flakiness costs > everyone a lot of time - from EWS being slower to analyzing induced memory > corruption crashes to fluctuations on performance metrics. I'd wager during the evenings when the pace of work drops off, it would've been acceptable to ping for a quick fix instead of insta-rolling out. > As we add more styles of testing, I expect that we will be only getting more > quick to roll back. I've been blowing this horn for years - EWS in its current state is harmful. See: The automation paradox. EWS is meant to prevent bad patches from going in, but then after patches go in we have a WHOLE CRAZY AMOUNT of additional reasons why a patch can be rolled out. As long as EWS doesn't prevent patches from going in that then might be instantly rolled out, that's a sign that our infrastructure is simply broken.

> it would've been acceptable to ping for a quick fix In fact, I pinged you on IRC, and you did not respond. I think that what you are asking for is a much higher level of service than is feasible late in the night - I generally only have a few minutes, so when I ping and don't get a fix, I may not be able to roll out later. > 1 - Made a quick fix for the GuardMalloc issues and then get 1 or 2 more bug fixes done Generally, making a fix while the tree is in a bad state is particularly advisable - it's too easy to do a less than a great job of fixing.

I meant "is NOT particularly advisable".