Bug 156472

Created attachment 276168 [details] Patch v1

Comment on attachment 276168 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=276168&action=review looks great! > Source/WebCore/Modules/indexeddb/IDBCursorWithValue.idl:33 > + [CallWith=ScriptState, ImplementationReturnType=JSValue] readonly attribute any value; It would be nice if the default return value were JSValue and we had to add something to make it Deprecated::ScriptValue.

(In reply to comment #2) > Comment on attachment 276168 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=276168&action=review > > looks great! > > > Source/WebCore/Modules/indexeddb/IDBCursorWithValue.idl:33 > > + [CallWith=ScriptState, ImplementationReturnType=JSValue] readonly attribute any value; > > It would be nice if the default return value were JSValue and we had to add > something to make it Deprecated::ScriptValue. I think this will be an awesome idea to implement in a patch dedicated to it. One patch that touches the code generator and many many many IDLs all at once.

Re-opened since this is blocked by bug 156483

Rolled out in r199323: <http://trac.webkit.org/changeset/199323>. Potential tests, which this change turns into crashes: https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r199312%20(4344)/results.html storage/indexeddb/cursor-continue-validity.html storage/indexeddb/cursor-request-cycle-private.html storage/indexeddb/cursor-value.html https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r199314%20(11347)/results.html imported/w3c/web-platform-tests/IndexedDB/idbindex-multientry-big.htm storage/indexeddb/cursor-request-cycle-private.html storage/indexeddb/cursor-value.html

I am pretty sure the reason these tests were failing is that we had JSValue data members in IDBAny and IDBCursor objects that are not on the stack. We need to use smart pointers instead.

Working on an updated version of this. There will need to be some "smart pointer to JSValue" changes (namely JSC::Strong<JSC::Unknown>) as well as some addOpaqueRoot / containsOpaqueRoot dancing.

(In reply to comment #8) > Working on an updated version of this. > > There will need to be some "smart pointer to JSValue" changes (namely > JSC::Strong<JSC::Unknown>) as well as some addOpaqueRoot / > containsOpaqueRoot dancing. Actually for now I'm just going to use JSC::Strong<JSC::Unknown>s wherever we used to use Deprecated::ScriptValue, as that is literally a zero-change in behavior. Anywhere use of D:SV might has caused a leak before, Strong<Unknown> will cause the same leak now. Fixing those potential leaks is something we can do in a followup.

Created attachment 276407 [details] New patch v1 Not marking for review yet until I run all layout tests in a few different configs.

(In reply to comment #9) > Actually for now I'm just going to use JSC::Strong<JSC::Unknown>s wherever > we used to use Deprecated::ScriptValue, as that is literally a zero-change > in behavior. Sounds exactly right. At some point, worth studying to find out whether there are any already-existing reference cycles.

(In reply to comment #11) > (In reply to comment #9) > > Actually for now I'm just going to use JSC::Strong<JSC::Unknown>s wherever > > we used to use Deprecated::ScriptValue, as that is literally a zero-change > > in behavior. > > Sounds exactly right. > > At some point, worth studying to find out whether there are any > already-existing reference cycles. It's trivial to create one with the existing code. e.g. - Iterate a cursor, and then set a property on the cursor's current value to be the cursor itself. cursor.value.foo = cursor; That's something that proper opaque roots will fix.

Obviously the new patch missed at least one niggling little detail. I'll revisit it soon, but it's not critical ATM.

I think the reference cycle issue is more important than the refactoring and cleanup being done by this patch. I’d also be happy to help resolve it. I wonder what the best way to write a regression test for the cycles is?

(In reply to comment #13) > Obviously the new patch missed at least one niggling little detail. I'll > revisit it soon, but it's not critical ATM. That one niggling detail is a missing include of "ScriptExecutionContext.h" in IDBBindingUtilities.cpp.

(In reply to comment #15) > (In reply to comment #13) > > Obviously the new patch missed at least one niggling little detail. I'll > > revisit it soon, but it's not critical ATM. > > That one niggling detail is a missing include of "ScriptExecutionContext.h" > in IDBBindingUtilities.cpp. And this demonstrates you went through the trouble of looking at the EWS failures, instead of just noting "EWS failed" and resolving to revisit later :)

(In reply to comment #14) > I think the reference cycle issue is more important than the refactoring and > cleanup being done by this patch. I’d also be happy to help resolve it. No argument there. I just did this because I was touching the binding utilities for other reasons, and I'd been wanting to. We have a bug for ref-cycles and leaks - https://bugs.webkit.org/show_bug.cgi?id=154015 - but no sub-task filed for this quite yet. > I wonder what the best way to write a regression test for the cycles is? That is, indeed, the challenge. There's a group of skipped IDB tests marked in TestExpectations with: # Relies on internals.observeGC That's how the chromium port + V8 detected these types of things back when they were part of the project. I don't think we have a replacement. Months ago when I discussed it with him, Geoff mentioned he thought having our own form of that would be both doable and desirable.

On a local branch I got rid of the IDBAny class entirely and replaced it with some custom bindings. I hope you land this soon; then I will merge and post that and try to whip it into shape.

(In reply to comment #18) > On a local branch I got rid of the IDBAny class entirely and replaced it > with some custom bindings. I hope you land this soon; then I will merge and > post that and try to whip it into shape. That's crazy exciting. I'll make sure I get this landed tomorrow before IDB'y things diverge much more.

Created attachment 276548 [details] New patch v2

Created attachment 276566 [details] New patch v3

I think our understanding of what went wrong first time is obviously right. Ran lots of tests locally, EWS passed, etc etc Let's land this.

Comment on attachment 276566 [details] New patch v3 Clearing flags on attachment: 276566 Committed r199643: <http://trac.webkit.org/changeset/199643>

Conrad has landed some "debug build" fix attempts in http://trac.webkit.org/changeset/199645 and http://trac.webkit.org/changeset/199646. I built debug locally, as did mac-debug EWS... =/

Those fixes are needed due to the combination of your patch and my patch. Separately each compiled. Together, they required these new includes. Not really "debug build specific" as far as I can tell.

(In reply to comment #25) > Those fixes are needed due to the combination of your patch and my patch. > Separately each compiled. Together, they required these new includes. > > Not really "debug build specific" as far as I can tell. I figured this out shortly after apologizing to Conrad for the breakage. Our two patches revealed an interesting race condition in the CQ bot - My build started before yours landed, which is why CQ cleared it. Annoying, but probably not all-too-common an occurrence.

Comment on attachment 276566 [details] New patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=276566&action=review > Source/WebCore/dom/ScriptExecutionContext.cpp:497 > + WorkerGlobalScope* workerGlobalScope = static_cast<WorkerGlobalScope*>(this); Why isn't this using a downcast<>() ?