Summary: | IndexedDB: Remove all index metadata records when deleting an index | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joshua Bell <jsbell> | ||||||||
Component: | New Bugs | Assignee: | Joshua Bell <jsbell> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | alecflett, dgrogan, tony, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Joshua Bell
2012-05-03 17:13:15 PDT
Created attachment 140129 [details]
Patch
Without this patch, loading storage/indexeddb/delete-index-crash.html in Chromium, quitting (or loading an interstitial page), then reloading storage/indexeddb/delete-index-crash.html will hit an ASSERT: ASSERTION FAILED: !metaDataKey.metaDataType() ../../third_party/WebKit/Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp(723) : virtual void WebCore::IDBLevelDBBackingStore::getIndexes(int64_t, int64_t, WTF::Vector<long int, 0ul>&, WTF::Vector<WTF::String, 0ul>&, WTF::Vector<WTF::String, 0ul>&, WTF::Vector<bool, 0ul>&, WTF::Vector<bool, 0ul>&) In theory, this should repro in DumpRenderTree - it would require dropping all references to the database and forcing GC to run so the database is dropped from the backing store map. I'm not seeing that happen with a LayoutTest when using window.location to navigate to an interstitial page, although it works in Chromium. We may be hitting something related to http://webkit.org/b/83074 or something DRT specific that's causing handles to be retained. I can land this then land a Chromium-side test, or hold off on landing until I can determine why DRT isn't releasing handles. Comment on attachment 140129 [details]
Patch
LGTM
Maybe we should try to get this in before the m20 freeze? That way, in the off chance that this IS causing docs problems, they'll get the fix a release early without us having to do a merge.
Comment on attachment 140129 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140129&action=review > Source/WebCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). > + > + No new tests. Issues does not repro in DumpRenderTree. Can you add a sentence or two explaining why you are making the change (hitting an assert). Please land a chromium side test after this gets rolled into chromium. You could maybe mention that here. > Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:1093 > + return encode(databaseId, objectStoreId, indexId, 255); What is 255? Should we make it a const somewhere? Created attachment 140308 [details]
Patch for landing
Comment on attachment 140308 [details] Patch for landing Rejecting attachment 140308 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/12565240 Created attachment 140310 [details]
Patch for landing
Comment on attachment 140310 [details] Patch for landing Clearing flags on attachment: 140310 Committed r116170: <http://trac.webkit.org/changeset/116170> All reviewed patches have been landed. Closing bug. |