Bug 48533 – IndexedDB: IDBRequest and IDBTransaction need massive changes

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Jeremy Orlow]

.

[Jeremy Orlow]

Created attachment 72209 [details]
patch

Still needs change log and there's one intermittent crash I'm trying to track down.

[Jeremy Orlow]

Created attachment 72332 [details]
ready

[Andrei Popescu]

> WebCore/dom/EventTarget.h

Why modify this file?

>  static Vector<RefPtr<IDBTransactionBackendInterface> >* m_transactions;

Hang on, did you work out why we crashed if this vector was storing raw pointers instead? If we have stale pointers in this vector, it seems we have transactions that are pending but have been released by the coordinator. That can't be.

[Jeremy Orlow]

(In reply to comment #3)
> > WebCore/dom/EventTarget.h
> 
> Why modify this file?

Just noticed it was inconsistent while I was reading the code.

> >  static Vector<RefPtr<IDBTransactionBackendInterface> >* m_transactions;
> 
> Hang on, did you work out why we crashed if this vector was storing raw pointers instead? If we have stale pointers in this vector, it seems we have transactions that are pending but have been released by the coordinator. That can't be.

I looked a bit, but didn't get very far.  I'm not particularly worried, but I guess I'll look a bit closer.

Basically the IDBTransaction goes out of scope almost immediately.  The IDBTransactionBackend should be getting added to the coordinator in its constructor though.  And that should be holding onto it until its completed.  And the destructor for IDBTransactionBackend should assert that it's finished when going away.  So unless something is massively going wrong, I think it's probably just already been aborted.  But yeah, it's worth tripple checking.

[Jeremy Orlow]

Created attachment 72508 [details]
all work up until this point

In case anyone wants to look at the whole set of patches together...

[Steve Block]

Comment on attachment 72332 [details]
ready

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

This looks OK, but I'm not familiar with the details, so it would be good to have somebody talk me through it.

Also, this is another big change - it would be helpful to split these up into smaller chunks where possible.

> WebCore/ChangeLog:16
> +        commands in the onsuccess handler that it intended to be atomic.

Can you make this more clear - too many 'it's for me to follow.

> WebCore/ChangeLog:18
> +        ignored. This is a bit overly conservitive, but we don't expect

conservative

> WebCore/ChangeLog:29
> +        change is that the behavior of IDBTransaction::objectStore more

is more

[Jeremy Orlow]

(In reply to comment #6)
> (From update of attachment 72332 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72332&action=review
> 
> This looks OK, but I'm not familiar with the details, so it would be good to have somebody talk me through it.
> 
> Also, this is another big change - it would be helpful to split these up into smaller chunks where possible.

Wait, did you look at the 2nd patch or the 3rd?  The second _was_ my attempt to split things up.  :-)  The third was just FYI and included the other patches you already reviewed.  I can probably split it a bit more if you really think it'd be helpful, but maybe just reviewing with you (on Friday?) would work too?

> > WebCore/ChangeLog:16
> > +        commands in the onsuccess handler that it intended to be atomic.
> 
> Can you make this more clear - too many 'it's for me to follow.
> 
> > WebCore/ChangeLog:18
> > +        ignored. This is a bit overly conservitive, but we don't expect
> 
> conservative
> 
> > WebCore/ChangeLog:29
> > +        change is that the behavior of IDBTransaction::objectStore more
> 
> is more

[Jeremy Orlow]

Created attachment 73096 [details]
Patch

[Steve Block]

Comment on attachment 73096 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73096&action=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.

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

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

Braces around && clause

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

> WebCore/storage/IDBTransaction.cpp:97
> +    // Future calls to objectStore should raise.

What objectStore - we're in the transaction?

> WebKit/chromium/src/IDBCallbacksProxy.cpp:-65
> -    m_callbacks.clear();

Why is this change useful?

[Jeremy Orlow]

Comment on attachment 73096 [details]
Patch

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.

[Andrei Popescu]

(In reply to comment #10)
> (From update of attachment 73096 [details])
> 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.
> 

Hmm, perhaps the better fix would be to call IDBPendingTransactionMonitor::removePendingTransaction()  in IDBTransaction::abort()?

That would solve the situation where one does:

var trans = db.transaction();
trans->abort();
window.gc();

As far as I understand, the problem is that, at the point of gc, the backend object is released so we're left with dereferencing a dangling pointer when we return to the event loop and iterate through the pending transactions and call transactionBackend->abort() on each of them. However, removing the transaction backend from the monitor during IDBTransaction::abort() would solve this problem in a nicer way?





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

[Jeremy Orlow]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 73096 [details] [details])
> > 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.
> > 
> 
> Hmm, perhaps the better fix would be to call IDBPendingTransactionMonitor::removePendingTransaction()  in IDBTransaction::abort()?
> 
> That would solve the situation where one does:
> 
> var trans = db.transaction();
> trans->abort();
> window.gc();
> 
> As far as I understand, the problem is that, at the point of gc, the backend object is released so we're left with dereferencing a dangling pointer when we return to the event loop and iterate through the pending transactions and call transactionBackend->abort() on each of them. However, removing the transaction backend from the monitor during IDBTransaction::abort() would solve this problem in a nicer way?

As we discussed in person:
It's quite likely that this would solve the problem and make it unnecessary to keep a RefPtr.  But what do we gain by not just using a RefPtr?  It's not a perf issue.  It _could_ mask other errors, but it's not clear that it matters: the worst that could happen is that we don't immediately kill an unused transaction and someone uses it shortly later.  But we already have other known bugs in this department and we've decided it just doesn't matter much.  But if we make it a raw ptr and have a bug, then it turns into memory corruptions and possibly security issues.

I think we should just leave it as is.

[Andrei Popescu]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (From update of attachment 73096 [details] [details] [details])
> > > 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.
> > > 
> > 
> > Hmm, perhaps the better fix would be to call IDBPendingTransactionMonitor::removePendingTransaction()  in IDBTransaction::abort()?
> > 
> > That would solve the situation where one does:
> > 
> > var trans = db.transaction();
> > trans->abort();
> > window.gc();
> > 
> > As far as I understand, the problem is that, at the point of gc, the backend object is released so we're left with dereferencing a dangling pointer when we return to the event loop and iterate through the pending transactions and call transactionBackend->abort() on each of them. However, removing the transaction backend from the monitor during IDBTransaction::abort() would solve this problem in a nicer way?
> 
> As we discussed in person:
> It's quite likely that this would solve the problem and make it unnecessary to keep a RefPtr.  But what do we gain by not just using a RefPtr?  It's not a perf issue.  It _could_ mask other errors, but it's not clear that it matters: the worst that could happen is that we don't immediately kill an unused transaction and someone uses it shortly later.  But we already have other known bugs in this department and we've decided it just doesn't matter much.  But if we make it a raw ptr and have a bug, then it turns into memory corruptions and possibly security issues.
> 
> I think we should just leave it as is.

Ok, I'm fine with using a RefPtr.

[Steve Block]

Comment on attachment 73096 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73096&action=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.
>>> 
>>> Hmm, perhaps the better fix would be to call IDBPendingTransactionMonitor::removePendingTransaction()  in IDBTransaction::abort()?
>>> 
>>> That would solve the situation where one does:
>>> 
>>> var trans = db.transaction();
>>> trans->abort();
>>> window.gc();
>>> 
>>> As far as I understand, the problem is that, at the point of gc, the backend object is released so we're left with dereferencing a dangling pointer when we return to the event loop and iterate through the pending transactions and call transactionBackend->abort() on each of them. However, removing the transaction backend from the monitor during IDBTransaction::abort() would solve this problem in a nicer way?
>> 
>> As we discussed in person:
>> It's quite likely that this would solve the problem and make it unnecessary to keep a RefPtr.  But what do we gain by not just using a RefPtr?  It's not a perf issue.  It _could_ mask other errors, but it's not clear that it matters: the worst that could happen is that we don't immediately kill an unused transaction and someone uses it shortly later.  But we already have other known bugs in this department and we've decided it just doesn't matter much.  But if we make it a raw ptr and have a bug, then it turns into memory corruptions and possibly security issues.
>> 
>> I think we should just leave it as is.
> 
> Ok, I'm fine with using a RefPtr.

OK, sure

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

You hit the case where a callback synchronously removes the context, causing a later callback to fail? If so, OK.

>>> WebCore/storage/IDBRequest.cpp:240
>>> +    if (!hasEventListeners() && m_pendingEvents.isEmpty() || !scriptExecutionContext()) {
>> 
>> Braces around && clause
> 
> I thought the style was to avoid ()'s unless necessary?

A quick look through the code suggests that otherwise.

[Jeremy Orlow]

(In reply to comment #14)
> (From update of attachment 73096 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73096&action=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.
> >>> 
> >>> Hmm, perhaps the better fix would be to call IDBPendingTransactionMonitor::removePendingTransaction()  in IDBTransaction::abort()?
> >>> 
> >>> That would solve the situation where one does:
> >>> 
> >>> var trans = db.transaction();
> >>> trans->abort();
> >>> window.gc();
> >>> 
> >>> As far as I understand, the problem is that, at the point of gc, the backend object is released so we're left with dereferencing a dangling pointer when we return to the event loop and iterate through the pending transactions and call transactionBackend->abort() on each of them. However, removing the transaction backend from the monitor during IDBTransaction::abort() would solve this problem in a nicer way?
> >> 
> >> As we discussed in person:
> >> It's quite likely that this would solve the problem and make it unnecessary to keep a RefPtr.  But what do we gain by not just using a RefPtr?  It's not a perf issue.  It _could_ mask other errors, but it's not clear that it matters: the worst that could happen is that we don't immediately kill an unused transaction and someone uses it shortly later.  But we already have other known bugs in this department and we've decided it just doesn't matter much.  But if we make it a raw ptr and have a bug, then it turns into memory corruptions and possibly security issues.
> >> 
> >> I think we should just leave it as is.
> > 
> > Ok, I'm fine with using a RefPtr.
> 
> OK, sure
> 
> >>>> 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.
> > 
> > 
> 
> You hit the case where a callback synchronously removes the context, causing a later callback to fail? If so, OK.
> 
> >>> WebCore/storage/IDBRequest.cpp:240
> >>> +    if (!hasEventListeners() && m_pendingEvents.isEmpty() || !scriptExecutionContext()) {
> >> 
> >> Braces around && clause
> > 
> > I thought the style was to avoid ()'s unless necessary?
> 
> A quick look through the code suggests that otherwise.

I checked the style guide and there seems to be no rule either way.

[Jeremy Orlow]

Moved m_selfRef = 0 down in IDBRequest and IDBTransaction.  Also switch from using contextDestroyed to stop to shut things down because killing the objects can end up touching a half dead scriptExecutionContext.  The only time stop() is not called immediately before contextDestroyed() is when there are pending XHRs that cause things to stay alive even though the page is unloaded, so stop might actually be the better place anyway.

[Jeremy Orlow]

Created attachment 77012 [details]
Patch

[WebKit Review Bot]

Attachment 77012 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7221071

[Jeremy Orlow]

Steve is out on vacation for several weeks.  Can anyone please take a final look at this so I can get it committed?

[Jeremy Orlow]

Created attachment 78679 [details]
Patch

[Jeremy Orlow]

Comment on attachment 78679 [details]
Patch

i'm going to basically re-write this and land as smaller bits as it's rotted quite a bit and we need some of the changes in this patch ASAP

[Joshua Bell]

Assuming this is dead like a ex-parrot.