Bug 48266 – Fix IndexedDB crashes

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

...

[Jeremy Orlow]

Created attachment 71866 [details]
patch

[Steve Block]

Comment on attachment 71866 [details]
patch

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

> LayoutTests/storage/indexeddb/keyrange.html:49
> +    shouldBe("rightFlags", openRight ? "keyRange.RIGHT_OPEN | keyRange.RIGHT_BOUND" : "keyRange.RIGHT_BOUND"); 

Intentional change?

> LayoutTests/storage/indexeddb/objectstore-basics.html:50
> +    debug("creatObjectStore():");

createObjectStore()

> LayoutTests/storage/indexeddb/objectstore-basics.html:68
> +    }

Seem to be some spurious diffs throughout this change. Is there any way you can remove them to make clear what's going on?

> WebCore/ChangeLog:11
> +        This is just a stop-gap move until we get it figured out upstream.

What do you mean by upstream?

[Jeremy Orlow]

Comment on attachment 71866 [details]
patch

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

>> LayoutTests/storage/indexeddb/keyrange.html:49
>> +    shouldBe("rightFlags", openRight ? "keyRange.RIGHT_OPEN | keyRange.RIGHT_BOUND" : "keyRange.RIGHT_BOUND"); 
> 
> Intentional change?

no

>> LayoutTests/storage/indexeddb/objectstore-basics.html:50
>> +    debug("creatObjectStore():");
> 
> createObjectStore()

done

>> LayoutTests/storage/indexeddb/objectstore-basics.html:68
>> +    }
> 
> Seem to be some spurious diffs throughout this change. Is there any way you can remove them to make clear what's going on?

I think Andrei introduced white space changes which my editor is fighting.  Sorry about that.  I'll try generating a diff without white spaces.

>> WebCore/ChangeLog:11
>> +        This is just a stop-gap move until we get it figured out upstream.
> 
> What do you mean by upstream?

In standards.  Changed.

[Jeremy Orlow]

Created attachment 71874 [details]
fixed

[Andrei Popescu]

> window.upper = testData.length-1;

here and below: add spaces around '-'


> if (m_transaction)
> IDBPendingTransactionMonitor::removePendingTransaction(m_transaction.get());

Why this change?

> while (!queue.isEmpty() && m_state != Finished) {

When can the state change to Finished during the loop below?

[Jeremy Orlow]

(In reply to comment #5)
> > window.upper = testData.length-1;
> 
> here and below: add spaces around '-'

Will do.

> > if (m_transaction)
> > IDBPendingTransactionMonitor::removePendingTransaction(m_transaction.get());
> 
> Why this change?
> 
> > while (!queue.isEmpty() && m_state != Finished) {
> 
> When can the state change to Finished during the loop below?

These are both described in the change log.

The former is because the whole page can shut down in the middle of a transaction and thus IDBRequest is deleted before the transaction finishes (but we should still make sure it's balanced).

The latter happens if it's aborted in between tasks.

Both have been seen in real life but couldn't be reliably reproduced, but seemed to go away when changed.

[Andrei Popescu]

(In reply to comment #6)
> (In reply to comment #5)
> > > window.upper = testData.length-1;
> > 
> > here and below: add spaces around '-'
> 
> Will do.
> 
> > > if (m_transaction)
> > > IDBPendingTransactionMonitor::removePendingTransaction(m_transaction.get());
> > 
> > Why this change?
> > 
> > > while (!queue.isEmpty() && m_state != Finished) {
> > 
> > When can the state change to Finished during the loop below?
> 
> These are both described in the change log.
> 
> The former is because the whole page can shut down in the middle of a transaction and thus IDBRequest is deleted before the transaction finishes (but we should still make sure it's balanced).
> 

But IDBRequest isn't just deleted. The GC will simply unref it. I think we should make sure that our previous assumption was really wrong and we're not keeping the request object alive while we have active transactions. 


> The latter happens if it's aborted in between tasks.
> 

When exactly can that happen? We're running a tight loop, so the only possibility is that one of the tasks calls abort() synchronously, but I can't think of any such case.

[Jeremy Orlow]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > > window.upper = testData.length-1;
> > > 
> > > here and below: add spaces around '-'
> > 
> > Will do.
> > 
> > > > if (m_transaction)
> > > > IDBPendingTransactionMonitor::removePendingTransaction(m_transaction.get());
> > > 
> > > Why this change?
> > > 
> > > > while (!queue.isEmpty() && m_state != Finished) {
> > > 
> > > When can the state change to Finished during the loop below?
> > 
> > These are both described in the change log.
> > 
> > The former is because the whole page can shut down in the middle of a transaction and thus IDBRequest is deleted before the transaction finishes (but we should still make sure it's balanced).
> > 
> 
> But IDBRequest isn't just deleted. The GC will simply unref it. I think we should make sure that our previous assumption was really wrong and we're not keeping the request object alive while we have active transactions. 

Yes, and when the last ref goes away, it'll be deleted.  What would be keeping it alive?

I love ASSERTs.  I don't think there's any way for us to do such a check though (especially in Chromium cause we're in different processes).  That's why we had that DCHECK to begin with.


> > The latter happens if it's aborted in between tasks.
> > 
> 
> When exactly can that happen? We're running a tight loop, so the only possibility is that one of the tasks calls abort() synchronously, but I can't think of any such case.

We do when we exceed quota or hit an error that'd leave things in an inconsistent state.

[Andrei Popescu]

LGTM

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > > window.upper = testData.length-1;
> > > > 
> > > > here and below: add spaces around '-'
> > > 
> > > Will do.
> > > 
> > > > > if (m_transaction)
> > > > > IDBPendingTransactionMonitor::removePendingTransaction(m_transaction.get());
> > > > 
> > > > Why this change?
> > > > 
> > > > > while (!queue.isEmpty() && m_state != Finished) {
> > > > 
> > > > When can the state change to Finished during the loop below?
> > > 
> > > These are both described in the change log.
> > > 
> > > The former is because the whole page can shut down in the middle of a transaction and thus IDBRequest is deleted before the transaction finishes (but we should still make sure it's balanced).
> > > 
> > 
> > But IDBRequest isn't just deleted. The GC will simply unref it. I think we should make sure that our previous assumption was really wrong and we're not keeping the request object alive while we have active transactions. 
> 
> Yes, and when the last ref goes away, it'll be deleted.  What would be keeping it alive?
>

Ok, I just wanted to make sure there's nothing that would be keeping it alive. It there isn't, it's ok to remove the assert.

 
> I love ASSERTs.  I don't think there's any way for us to do such a check though (especially in Chromium cause we're in different processes).  That's why we had that DCHECK to begin with.
> 

Er, which DCHECK?


> 
> > > The latter happens if it's aborted in between tasks.
> > > 
> > 
> > When exactly can that happen? We're running a tight loop, so the only possibility is that one of the tasks calls abort() synchronously, but I can't think of any such case.
> 
> We do when we exceed quota or hit an error that'd leave things in an inconsistent state.

Oh, I see, we added that after we wrote the code for the transaction task processing. Makes sense.

[Jeremy Orlow]

(In reply to comment #9)
> LGTM
> 
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > (In reply to comment #5)
> > > > > > window.upper = testData.length-1;
> > > > > 
> > > > > here and below: add spaces around '-'
> > > > 
> > > > Will do.
> > > > 
> > > > > > if (m_transaction)
> > > > > > IDBPendingTransactionMonitor::removePendingTransaction(m_transaction.get());
> > > > > 
> > > > > Why this change?
> > > > > 
> > > > > > while (!queue.isEmpty() && m_state != Finished) {
> > > > > 
> > > > > When can the state change to Finished during the loop below?
> > > > 
> > > > These are both described in the change log.
> > > > 
> > > > The former is because the whole page can shut down in the middle of a transaction and thus IDBRequest is deleted before the transaction finishes (but we should still make sure it's balanced).
> > > > 
> > > 
> > > But IDBRequest isn't just deleted. The GC will simply unref it. I think we should make sure that our previous assumption was really wrong and we're not keeping the request object alive while we have active transactions. 
> > 
> > Yes, and when the last ref goes away, it'll be deleted.  What would be keeping it alive?
> >
> 
> Ok, I just wanted to make sure there's nothing that would be keeping it alive. It there isn't, it's ok to remove the assert.

Note that the assert wasn't just removed: it was replaced by logic that does the correct thing in such cases.

> > I love ASSERTs.  I don't think there's any way for us to do such a check though (especially in Chromium cause we're in different processes).  That's why we had that DCHECK to begin with.
> > 
> 
> Er, which DCHECK?

Err...assert:  ASSERT(!m_transaction);

> > > > The latter happens if it's aborted in between tasks.
> > > > 
> > > 
> > > When exactly can that happen? We're running a tight loop, so the only possibility is that one of the tasks calls abort() synchronously, but I can't think of any such case.
> > 
> > We do when we exceed quota or hit an error that'd leave things in an inconsistent state.
> 
> Oh, I see, we added that after we wrote the code for the transaction task processing. Makes sense.

Yup.

[Andrei Popescu]

(In reply to comment #10)
> (In reply to comment #9)
> > LGTM
> > 
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > (In reply to comment #6)
> > > > > (In reply to comment #5)
> > > > > > > window.upper = testData.length-1;
> > > > > > 
> > > > > > here and below: add spaces around '-'
> > > > > 
> > > > > Will do.
> > > > > 
> > > > > > > if (m_transaction)
> > > > > > > IDBPendingTransactionMonitor::removePendingTransaction(m_transaction.get());
> > > > > > 
> > > > > > Why this change?
> > > > > > 
> > > > > > > while (!queue.isEmpty() && m_state != Finished) {
> > > > > > 
> > > > > > When can the state change to Finished during the loop below?
> > > > > 
> > > > > These are both described in the change log.
> > > > > 
> > > > > The former is because the whole page can shut down in the middle of a transaction and thus IDBRequest is deleted before the transaction finishes (but we should still make sure it's balanced).
> > > > > 
> > > > 
> > > > But IDBRequest isn't just deleted. The GC will simply unref it. I think we should make sure that our previous assumption was really wrong and we're not keeping the request object alive while we have active transactions. 
> > > 
> > > Yes, and when the last ref goes away, it'll be deleted.  What would be keeping it alive?
> > >
> > 
> > Ok, I just wanted to make sure there's nothing that would be keeping it alive. It there isn't, it's ok to remove the assert.
> 
> Note that the assert wasn't just removed: it was replaced by logic that does the correct thing in such cases.
> 

Actually, thinking about this, I think this logic isn't right. The transaction isn't pending anymore so the code you added will probably be a no-op. Instead you should just do m_transaction->abort().

[Jeremy Orlow]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > LGTM
> > > 
> > > (In reply to comment #8)
> > > > (In reply to comment #7)
> > > > > (In reply to comment #6)
> > > > > > (In reply to comment #5)
> > > > > > > > window.upper = testData.length-1;
> > > > > > > 
> > > > > > > here and below: add spaces around '-'
> > > > > > 
> > > > > > Will do.
> > > > > > 
> > > > > > > > if (m_transaction)
> > > > > > > > IDBPendingTransactionMonitor::removePendingTransaction(m_transaction.get());
> > > > > > > 
> > > > > > > Why this change?
> > > > > > > 
> > > > > > > > while (!queue.isEmpty() && m_state != Finished) {
> > > > > > > 
> > > > > > > When can the state change to Finished during the loop below?
> > > > > > 
> > > > > > These are both described in the change log.
> > > > > > 
> > > > > > The former is because the whole page can shut down in the middle of a transaction and thus IDBRequest is deleted before the transaction finishes (but we should still make sure it's balanced).
> > > > > > 
> > > > > 
> > > > > But IDBRequest isn't just deleted. The GC will simply unref it. I think we should make sure that our previous assumption was really wrong and we're not keeping the request object alive while we have active transactions. 
> > > > 
> > > > Yes, and when the last ref goes away, it'll be deleted.  What would be keeping it alive?
> > > >
> > > 
> > > Ok, I just wanted to make sure there's nothing that would be keeping it alive. It there isn't, it's ok to remove the assert.
> > 
> > Note that the assert wasn't just removed: it was replaced by logic that does the correct thing in such cases.
> > 
> 
> Actually, thinking about this, I think this logic isn't right. The transaction isn't pending anymore so the code you added will probably be a no-op. Instead you should just do m_transaction->abort().

As we discussed, abort is not the right course of action, but neither is removePendingTransaction.  What we really want is didCompletePendingTasks.  Will change.

[Jeremy Orlow]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > LGTM
> > > > 
> > > > (In reply to comment #8)
> > > > > (In reply to comment #7)
> > > > > > (In reply to comment #6)
> > > > > > > (In reply to comment #5)
> > > > > > > > > window.upper = testData.length-1;
> > > > > > > > 
> > > > > > > > here and below: add spaces around '-'
> > > > > > > 
> > > > > > > Will do.
> > > > > > > 
> > > > > > > > > if (m_transaction)
> > > > > > > > > IDBPendingTransactionMonitor::removePendingTransaction(m_transaction.get());
> > > > > > > > 
> > > > > > > > Why this change?
> > > > > > > > 
> > > > > > > > > while (!queue.isEmpty() && m_state != Finished) {
> > > > > > > > 
> > > > > > > > When can the state change to Finished during the loop below?
> > > > > > > 
> > > > > > > These are both described in the change log.
> > > > > > > 
> > > > > > > The former is because the whole page can shut down in the middle of a transaction and thus IDBRequest is deleted before the transaction finishes (but we should still make sure it's balanced).
> > > > > > > 
> > > > > > 
> > > > > > But IDBRequest isn't just deleted. The GC will simply unref it. I think we should make sure that our previous assumption was really wrong and we're not keeping the request object alive while we have active transactions. 
> > > > > 
> > > > > Yes, and when the last ref goes away, it'll be deleted.  What would be keeping it alive?
> > > > >
> > > > 
> > > > Ok, I just wanted to make sure there's nothing that would be keeping it alive. It there isn't, it's ok to remove the assert.
> > > 
> > > Note that the assert wasn't just removed: it was replaced by logic that does the correct thing in such cases.
> > > 
> > 
> > Actually, thinking about this, I think this logic isn't right. The transaction isn't pending anymore so the code you added will probably be a no-op. Instead you should just do m_transaction->abort().
> 
> As we discussed, abort is not the right course of action, but neither is removePendingTransaction.  What we really want is didCompletePendingTasks.  Will change.

Crap....that's not true either.

I just realized what's triggering this: it happens when we create IDBRequest objects but then never use them because the backend immediately throws.  Thus I think this comment+assert is completely invalid and there's no simple way to handle these checks.  I think we should probably just remove it.

[Jeremy Orlow]

Created attachment 71899 [details]
another fix

[Andrei Popescu]

(In reply to comment #14)
> Created an attachment (id=71899) [details]
> another fix

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > (In reply to comment #9)
> > > > > LGTM
> > > > > 
> > > > > (In reply to comment #8)
> > > > > > (In reply to comment #7)
> > > > > > > (In reply to comment #6)
> > > > > > > > (In reply to comment #5)
> > > > > > > > > > window.upper = testData.length-1;
> > > > > > > > > 
> > > > > > > > > here and below: add spaces around '-'
> > > > > > > > 
> > > > > > > > Will do.
> > > > > > > > 
> > > > > > > > > > if (m_transaction)
> > > > > > > > > > IDBPendingTransactionMonitor::removePendingTransaction(m_transaction.get());
> > > > > > > > > 
> > > > > > > > > Why this change?
> > > > > > > > > 
> > > > > > > > > > while (!queue.isEmpty() && m_state != Finished) {
> > > > > > > > > 
> > > > > > > > > When can the state change to Finished during the loop below?
> > > > > > > > 
> > > > > > > > These are both described in the change log.
> > > > > > > > 
> > > > > > > > The former is because the whole page can shut down in the middle of a transaction and thus IDBRequest is deleted before the transaction finishes (but we should still make sure it's balanced).
> > > > > > > > 
> > > > > > > 
> > > > > > > But IDBRequest isn't just deleted. The GC will simply unref it. I think we should make sure that our previous assumption was really wrong and we're not keeping the request object alive while we have active transactions. 
> > > > > > 
> > > > > > Yes, and when the last ref goes away, it'll be deleted.  What would be keeping it alive?
> > > > > >
> > > > > 
> > > > > Ok, I just wanted to make sure there's nothing that would be keeping it alive. It there isn't, it's ok to remove the assert.
> > > > 
> > > > Note that the assert wasn't just removed: it was replaced by logic that does the correct thing in such cases.
> > > > 
> > > 
> > > Actually, thinking about this, I think this logic isn't right. The transaction isn't pending anymore so the code you added will probably be a no-op. Instead you should just do m_transaction->abort().
> > 
> > As we discussed, abort is not the right course of action, but neither is removePendingTransaction.  What we really want is didCompletePendingTasks.  Will change.
> 
> Crap....that's not true either.
> 
> I just realized what's triggering this: it happens when we create IDBRequest objects but then never use them because the backend immediately throws.  Thus I think this comment+assert is completely invalid and there's no simple way to handle these checks.  I think we should probably just remove it.

Yeah...I guess one way to handle it would be to somehow notify the IDBRequest object at the point where the backend throws. Not sure if it's worth doing...

[Jeremy Orlow]

(In reply to comment #15)
> (In reply to comment #14)
> > Created an attachment (id=71899) [details] [details]
> > another fix
> 
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > (In reply to comment #10)
> > > > > (In reply to comment #9)
> > > > > > LGTM
> > > > > > 
> > > > > > (In reply to comment #8)
> > > > > > > (In reply to comment #7)
> > > > > > > > (In reply to comment #6)
> > > > > > > > > (In reply to comment #5)
> > > > > > > > > > > window.upper = testData.length-1;
> > > > > > > > > > 
> > > > > > > > > > here and below: add spaces around '-'
> > > > > > > > > 
> > > > > > > > > Will do.
> > > > > > > > > 
> > > > > > > > > > > if (m_transaction)
> > > > > > > > > > > IDBPendingTransactionMonitor::removePendingTransaction(m_transaction.get());
> > > > > > > > > > 
> > > > > > > > > > Why this change?
> > > > > > > > > > 
> > > > > > > > > > > while (!queue.isEmpty() && m_state != Finished) {
> > > > > > > > > > 
> > > > > > > > > > When can the state change to Finished during the loop below?
> > > > > > > > > 
> > > > > > > > > These are both described in the change log.
> > > > > > > > > 
> > > > > > > > > The former is because the whole page can shut down in the middle of a transaction and thus IDBRequest is deleted before the transaction finishes (but we should still make sure it's balanced).
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > But IDBRequest isn't just deleted. The GC will simply unref it. I think we should make sure that our previous assumption was really wrong and we're not keeping the request object alive while we have active transactions. 
> > > > > > > 
> > > > > > > Yes, and when the last ref goes away, it'll be deleted.  What would be keeping it alive?
> > > > > > >
> > > > > > 
> > > > > > Ok, I just wanted to make sure there's nothing that would be keeping it alive. It there isn't, it's ok to remove the assert.
> > > > > 
> > > > > Note that the assert wasn't just removed: it was replaced by logic that does the correct thing in such cases.
> > > > > 
> > > > 
> > > > Actually, thinking about this, I think this logic isn't right. The transaction isn't pending anymore so the code you added will probably be a no-op. Instead you should just do m_transaction->abort().
> > > 
> > > As we discussed, abort is not the right course of action, but neither is removePendingTransaction.  What we really want is didCompletePendingTasks.  Will change.
> > 
> > Crap....that's not true either.
> > 
> > I just realized what's triggering this: it happens when we create IDBRequest objects but then never use them because the backend immediately throws.  Thus I think this comment+assert is completely invalid and there's no simple way to handle these checks.  I think we should probably just remove it.
> 
> Yeah...I guess one way to handle it would be to somehow notify the IDBRequest object at the point where the backend throws. Not sure if it's worth doing...

Doing this from the backend would be messy and require a lot of plumbing or do things differently in Chromium and normal WebKit--neither of which I want to do if possible.

The best thing I can think of is to do it from the frontend (have it check for exceptions and tell the IDBRequest it happened).  This seems like a lot of ugly boilerplate code.

At the end of the day, all we're losing is an ASSERT.  Is that really worth the effort?  If so, maybe we should instead think of how to fundamentally re-architect this.

[Steve Block]

Comment on attachment 71899 [details]
another fix

r=me

[Andrei Popescu]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > Created an attachment (id=71899) [details] [details] [details]
> > > another fix
> > 
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > (In reply to comment #11)
> > > > > (In reply to comment #10)
> > > > > > (In reply to comment #9)
> > > > > > > LGTM
> > > > > > > 
> > > > > > > (In reply to comment #8)
> > > > > > > > (In reply to comment #7)
> > > > > > > > > (In reply to comment #6)
> > > > > > > > > > (In reply to comment #5)
> > > > > > > > > > > > window.upper = testData.length-1;
> > > > > > > > > > > 
> > > > > > > > > > > here and below: add spaces around '-'
> > > > > > > > > > 
> > > > > > > > > > Will do.
> > > > > > > > > > 
> > > > > > > > > > > > if (m_transaction)
> > > > > > > > > > > > IDBPendingTransactionMonitor::removePendingTransaction(m_transaction.get());
> > > > > > > > > > > 
> > > > > > > > > > > Why this change?
> > > > > > > > > > > 
> > > > > > > > > > > > while (!queue.isEmpty() && m_state != Finished) {
> > > > > > > > > > > 
> > > > > > > > > > > When can the state change to Finished during the loop below?
> > > > > > > > > > 
> > > > > > > > > > These are both described in the change log.
> > > > > > > > > > 
> > > > > > > > > > The former is because the whole page can shut down in the middle of a transaction and thus IDBRequest is deleted before the transaction finishes (but we should still make sure it's balanced).
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > But IDBRequest isn't just deleted. The GC will simply unref it. I think we should make sure that our previous assumption was really wrong and we're not keeping the request object alive while we have active transactions. 
> > > > > > > > 
> > > > > > > > Yes, and when the last ref goes away, it'll be deleted.  What would be keeping it alive?
> > > > > > > >
> > > > > > > 
> > > > > > > Ok, I just wanted to make sure there's nothing that would be keeping it alive. It there isn't, it's ok to remove the assert.
> > > > > > 
> > > > > > Note that the assert wasn't just removed: it was replaced by logic that does the correct thing in such cases.
> > > > > > 
> > > > > 
> > > > > Actually, thinking about this, I think this logic isn't right. The transaction isn't pending anymore so the code you added will probably be a no-op. Instead you should just do m_transaction->abort().
> > > > 
> > > > As we discussed, abort is not the right course of action, but neither is removePendingTransaction.  What we really want is didCompletePendingTasks.  Will change.
> > > 
> > > Crap....that's not true either.
> > > 
> > > I just realized what's triggering this: it happens when we create IDBRequest objects but then never use them because the backend immediately throws.  Thus I think this comment+assert is completely invalid and there's no simple way to handle these checks.  I think we should probably just remove it.
> > 
> > Yeah...I guess one way to handle it would be to somehow notify the IDBRequest object at the point where the backend throws. Not sure if it's worth doing...
> 
> Doing this from the backend would be messy and require a lot of plumbing or do things differently in Chromium and normal WebKit--neither of which I want to do if possible.
> 
> The best thing I can think of is to do it from the frontend (have it check for exceptions and tell the IDBRequest it happened).  This seems like a lot of ugly boilerplate code.
> 
> At the end of the day, all we're losing is an ASSERT.  Is that really worth the effort? 

Probably not.

[Jeremy Orlow]

Comment on attachment 71899 [details]
another fix

Clearing flags on attachment: 71899

Committed r70531: <http://trac.webkit.org/changeset/70531>

[Jeremy Orlow]

All reviewed patches have been landed.  Closing bug.