[webkit-reviews] review denied: [Bug 48533] IDBRequest and IDBTransaction need massive changes : [Attachment 73096] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Jeremy Orlow
<jorlow at chromium.org>'s request for review:
Bug 48533: IDBRequest and IDBTransaction need massive changes
https://bugs.webkit.org/show_bug.cgi?id=48533

Attachment 73096: Patch
https://bugs.webkit.org/attachment.cgi?id=73096&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=73096&action=review

Please take a look at my comments.  I'm pretty sure m_selfRef = 0 moving is all
I need to do.  If so, I'll rebase, make the change, and upload a version for a
final review.

>> WebCore/storage/IDBPendingTransactionMonitor.cpp:34
>> +Vector<RefPtr<IDBTransactionBackendInterface> >*
IDBPendingTransactionMonitor::m_transactions = 0;
> 
> Does the fact that we need a RefPtr here to avoid a crash imply that we've
missed a call to IDBPendingTransactionMonitor::removePendingTransaction()
somewhere? I can't see why else it's needed.

If a transaction is aborted and a gc happens before returning control from
JavaScript, then the transaction monitor can have items in it which don't have
any other live reference.

>> WebCore/storage/IDBRequest.cpp:199
>> +	    if (!scriptExecutionContext()) {
> 
> Is there a LayoutTest for this case, where a callback deletes the
ScriptExecutionContext?
> 
> Do we really need to check the context before each callback, or is it OK to
check once before the loop? Can a callback delete the context synchronously?

I definitely hit this while testing.  I believe the attached layout test did,
but given that I wrote this a patch a month ago, the details are a bit fuzzy.

>> WebCore/storage/IDBRequest.cpp:240
>> +	if (!hasEventListeners() && m_pendingEvents.isEmpty() ||
!scriptExecutionContext()) {
> 
> Braces around && clause

I thought the style was to avoid ()'s unless necessary?

>> WebCore/storage/IDBRequest.cpp:243
>> +	    m_timer.stop();
> 
> Is it safe to do this after we've released our own ref? It could be the last.


Very good point.  m_selfRef should definitely be moved down.

>> WebCore/storage/IDBTransaction.cpp:97
>> +	// Future calls to objectStore should raise.
> 
> What objectStore - we're in the transaction?

IDBTransaction::objectStore()

>> WebKit/chromium/src/IDBCallbacksProxy.cpp:-65
>> -	m_callbacks.clear();
> 
> Why is this change useful?

Consistency.  It was making Chromium behave differently than other ports for no
reason.