| Summary: | Fix IndexedDB crashes | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jeremy Orlow <jorlow> | ||||||||
| Component: | New Bugs | Assignee: | Jeremy Orlow <jorlow> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | andreip, hans, steveblock | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | OS X 10.5 | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 48067 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Jeremy Orlow
2010-10-25 14:28:13 PDT
Created attachment 71866 [details]
patch
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? 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. Created attachment 71874 [details]
fixed
> 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? (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. (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. (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. 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. (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. (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(). (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. (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. Created attachment 71899 [details]
another fix
(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... (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. Comment on attachment 71899 [details]
another fix
r=me
(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. Comment on attachment 71899 [details] another fix Clearing flags on attachment: 71899 Committed r70531: <http://trac.webkit.org/changeset/70531> All reviewed patches have been landed. Closing bug. |