Bug 152397 – Modern IDB: Refactor open/delete requests to exist in the same queue

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

[Brady Eidson]

Modern IDB: Refactor open/delete requests to exist in the same queue

This is a straight "no behavior change" patch, where all existing tests continue to pass.

But it is necessary to fix tests currently skipped.

[Brady Eidson]

Created attachment 267583 [details]
Patch v1

[Brady Eidson]

https://trac.webkit.org/changeset/194241

[Alexey Proskuryakov]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 152415

[Brady Eidson]

(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.

[Brady Eidson]

(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*

[Brady Eidson]

Created attachment 267610 [details]
Patch for landing

[Brady Eidson]

(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.

[Alexey Proskuryakov]

> 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.

[Alexey Proskuryakov]

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

[WebKit Commit Bot]

Comment on attachment 267610 [details]
Patch for landing

Clearing flags on attachment: 267610

Committed r194263: <http://trac.webkit.org/changeset/194263>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brady Eidson]

(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.

[Alexey Proskuryakov]

> 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.

[Alexey Proskuryakov]

I meant "is NOT particularly advisable".