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.
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
(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 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
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
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
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 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
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
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
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
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.
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()).
2019-02-18 21:02 PST, Sihui Liu
2019-02-18 22:43 PST, EWS Watchlist
2019-02-19 11:30 PST, Sihui Liu
2019-03-07 16:03 PST, Sihui Liu
2019-03-07 16:37 PST, EWS Watchlist
2019-03-07 16:59 PST, EWS Watchlist
2019-03-07 17:42 PST, EWS Watchlist
2019-03-07 23:49 PST, EWS Watchlist
2019-03-08 01:07 PST, Sihui Liu
2019-03-08 02:28 PST, EWS Watchlist
2019-03-08 02:34 PST, EWS Watchlist
2019-03-08 03:08 PST, EWS Watchlist
2019-03-08 09:25 PST, EWS Watchlist
2019-03-11 15:03 PDT, EWS Watchlist
2019-03-14 11:27 PDT, Sihui Liu
2019-03-14 18:46 PDT, Sihui Liu