Description
Sihui Liu
2019-02-18 20:55:56 PST
Created attachment 362367 [details]
Patch
Comment on attachment 362367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362367&action=review > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:-366 > - VM& vm = context->vm(); > - JSLockHolder lock(vm); I think we may still need this lock of the JS VM, since assigning { } to m_resultWrapper may call weakClearSlowCase() and use the JS VM. > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:-384 > - JSLockHolder lock(vm); Ditto. > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:-402 > - JSLockHolder lock(vm); Ditto. > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:-414 > - Maybe add a lock here? > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:-434 > - JSLockHolder lock(vm); Ditto. > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:427 > + JSC::JSValue value = m_resultWrapper; > + m_cursorWrapper = value; Why do we want our cursor to hold our result here? Probably worth adding an assignment operator to JSValueInWrappedObject to make this more convenient. Comment on attachment 362367 [details] Patch Attachment 362367 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11200591 New failing tests: storage/indexeddb/connection-leak.html storage/indexeddb/request-leak.html storage/indexeddb/connection-leak-private.html storage/indexeddb/delete-closed-database-object-private.html storage/indexeddb/delete-closed-database-object.html storage/indexeddb/request-leak-private.html storage/indexeddb/cursor-request-cycle.html storage/indexeddb/cursor-leak.html storage/indexeddb/cursor-request-cycle-private.html storage/indexeddb/modern/gc-closes-database-private.html storage/indexeddb/cursor-leak-private.html Created attachment 362368 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 362402 [details]
Patch
(In reply to Geoffrey Garen from comment #2) > Comment on attachment 362367 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362367&action=review > > > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:-366 > > - VM& vm = context->vm(); > > - JSLockHolder lock(vm); > > I think we may still need this lock of the JS VM, since assigning { } to > m_resultWrapper may call weakClearSlowCase() and use the JS VM. > Add a bool in JSValueInWrappedObject for explicitly invalidate the value, so we don't need the lock any more. > > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:427 > > + JSC::JSValue value = m_resultWrapper; > > + m_cursorWrapper = value; > > Why do we want our cursor to hold our result here? > Added some explanation in the changelog. IDBCursor is a little special in that if the advance operation is not completed yet, we need to return error as result; and after the operation on success, we want to return the same JSIDBCursor object as before the advance operation. So I cached the result before the operation and restore it after success. > Probably worth adding an assignment operator to JSValueInWrappedObject to > make this more convenient. Sure. Comment on attachment 362402 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362402&action=review > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:425 > + if (m_resultWrapper && !m_resultWrapper.isStale()) I think you added the concept of isStale() to avoid acquiring the JS lock. I'm worried that it's erorr-prone to have two null states in an object: both operator! null and isStale null. A programmer might forget to check both states. Also, I just noticed that this object, and probably other objects related to it, need the JS lock in their destructors too. I think we probably need to just acquire the JS lock. Another option might be to build this null state into JSValueInWrappedObject and make it automatic for things like assignment and operator bool, for convenience. But that won't solve the destructor problem :(. Created attachment 363946 [details]
Patch
Comment on attachment 363946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363946&action=review r=me > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:350 > +void IDBCursor::clearRequest() I think we need a lock here too because we clear wrappers. Comment on attachment 363946 [details] Patch Attachment 363946 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11420183 Number of test failures exceeded the failure limit. Created attachment 363955 [details]
Archive of layout-test-results from ews102 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 363946 [details] Patch Attachment 363946 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11420429 Number of test failures exceeded the failure limit. Created attachment 363959 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 363946 [details] Patch Attachment 363946 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11420880 Number of test failures exceeded the failure limit. Created attachment 363964 [details]
Archive of layout-test-results from ews112 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 363946 [details] Patch Attachment 363946 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11424049 New failing tests: imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html compositing/geometry/ancestor-clip-change.html css3/filters/blur-various-radii.html Created attachment 363992 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 363996 [details]
Patch
Comment on attachment 363996 [details] Patch Attachment 363996 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11424857 New failing tests: storage/indexeddb/pending-version-change-on-exit-private.html storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange.html storage/indexeddb/pending-version-change-stuck-works-with-terminate-private.html storage/indexeddb/pending-version-change-stuck.html http/tests/security/cross-origin-worker-indexeddb.html storage/indexeddb/modern/blob-simple.html imported/w3c/web-platform-tests/IndexedDB/idbcursor-advance-continue-async.htm storage/indexeddb/objectstore-basics.html storage/indexeddb/opencursor-key-private.html storage/indexeddb/pending-version-change-on-exit.html storage/indexeddb/basics.html storage/indexeddb/transaction-complete-workers.html storage/indexeddb/pending-version-change-stuck-works-with-terminate.html storage/indexeddb/modern/worker-transaction-open-after-worker-stop.html imported/w3c/IndexedDB-private-browsing/idbcursor-advance-continue-async.html storage/indexeddb/pending-version-change-stuck-private.html Created attachment 363999 [details]
Archive of layout-test-results from ews101 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 363996 [details] Patch Attachment 363996 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11424863 New failing tests: storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange.html storage/indexeddb/pending-version-change-stuck.html storage/indexeddb/pending-version-change-stuck-works-with-terminate-private.html storage/indexeddb/opencursor-key-private.html storage/indexeddb/modern/blob-simple.html storage/indexeddb/pending-version-change-stuck-works-with-terminate.html imported/w3c/web-platform-tests/IndexedDB/idbcursor-advance-continue-async.htm storage/indexeddb/pending-version-change-on-exit.html http/wpt/cache-storage/cache-quota.any.html storage/indexeddb/objectstore-basics.html storage/indexeddb/modern/worker-transaction-open-after-worker-stop.html storage/indexeddb/basics.html storage/indexeddb/transaction-complete-workers.html storage/indexeddb/cursor-advance.html http/tests/security/cross-origin-worker-indexeddb.html imported/w3c/IndexedDB-private-browsing/idbcursor-advance-continue-async.html storage/indexeddb/pending-activity.html storage/indexeddb/index-basics.html storage/indexeddb/dont-wedge-private.html Created attachment 364000 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 363996 [details] Patch Attachment 363996 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11424885 Number of test failures exceeded the failure limit. Created attachment 364002 [details]
Archive of layout-test-results from ews115 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 363996 [details] Patch Attachment 363996 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11426624 New failing tests: imported/w3c/web-platform-tests/webrtc/simplecall.https.html storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange.html storage/indexeddb/pending-version-change-stuck-works-with-terminate-private.html storage/indexeddb/pending-version-change-stuck.html storage/indexeddb/modern/blob-simple.html storage/indexeddb/pending-version-change-stuck-works-with-terminate.html imported/w3c/web-platform-tests/IndexedDB/idbcursor-advance-continue-async.htm storage/indexeddb/objectstore-basics.html storage/indexeddb/opencursor-key-private.html storage/indexeddb/pending-version-change-on-exit.html storage/indexeddb/basics.html storage/indexeddb/transaction-complete-workers.html storage/indexeddb/cursor-advance.html storage/indexeddb/modern/worker-transaction-open-after-worker-stop.html imported/w3c/IndexedDB-private-browsing/idbcursor-advance-continue-async.html storage/indexeddb/pending-activity.html storage/indexeddb/index-basics.html storage/indexeddb/dont-wedge-private.html Created attachment 364018 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 363996 [details] Patch Attachment 363996 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11460454 New failing tests: storage/indexeddb/pending-version-change-on-exit-private.html storage/indexeddb/pending-version-change-stuck-works-with-terminate-private.html storage/indexeddb/modern/worker-getall.html storage/indexeddb/modern/blob-simple-workers.html storage/indexeddb/pending-version-change-stuck-private.html storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange-workers.html storage/indexeddb/dont-commit-on-blocked-private.html storage/indexeddb/pending-version-change-stuck.html storage/indexeddb/pending-version-change-on-exit.html storage/indexeddb/basics-workers.html storage/indexeddb/dont-commit-on-blocked.html storage/indexeddb/transaction-complete-workers.html storage/indexeddb/pending-version-change-stuck-works-with-terminate.html storage/indexeddb/transaction-complete-workers-private.html storage/indexeddb/modern/worker-transaction-open-after-worker-stop.html storage/indexeddb/pending-activity-workers.html storage/indexeddb/cursor-advance-workers.html storage/indexeddb/index-basics-workers.html storage/indexeddb/open-twice-workers.html Created attachment 364294 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Seems like you've discovered a pre-existing use-after-free. We either need to perform this shutdown in a slightly different order, such that script() and script()->vm() are not null during shutdown, or we need another way to access VM* from IDB destructors. Created attachment 364667 [details]
Patch
Comment on attachment 364667 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364667&action=review r=me with two suggested fixes > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:155 > + if (!m_contextStopped) > + clearWrappers(); I think you can remove this. Let's discuss. > Source/WebCore/bindings/js/JSValueInWrappedObject.h:39 > + JSValueInWrappedObject(JSValueInWrappedObject&); Should this be const JSValueInWrappedObject&? I don't think this is a copy constructor if you forget the const. Comment on attachment 364667 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364667&action=review > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:297 > + auto* context = scriptExecutionContext(); > + VM& vm = context->vm(); > + JSLockHolder lock(vm); For clarity, we might as well move this lock into the the callee (clearWrappers()). Created attachment 364749 [details]
Patch for landing
Comment on attachment 364749 [details] Patch for landing Clearing flags on attachment: 364749 Committed r242986: <https://trac.webkit.org/changeset/242986> All reviewed patches have been landed. Closing bug. |