Bug 156472 – Clean up IDBBindingUtilities

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Clean up IDBBindingUtilities massively

[Brady Eidson]

Created attachment 276168 [details]
Patch v1

[Alex Christensen]

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.

[Brady Eidson]

(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.

[Brady Eidson]

http://trac.webkit.org/changeset/199310

[WebKit Commit Bot]

Re-opened since this is blocked by bug 156483

[Jiewen Tan]

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

[Darin Adler]

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.

[Brady Eidson]

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.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 276407 [details]
New patch v1

Not marking for review yet until I run all layout tests in a few different configs.

[Darin Adler]

(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.

[Brady Eidson]

(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.

[Brady Eidson]

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

[Darin Adler]

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?

[Darin Adler]

(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.

[Brady Eidson]

(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  :)

[Brady Eidson]

(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.

[Darin Adler]

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.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 276548 [details]
New patch v2

[Brady Eidson]

Created attachment 276566 [details]
New patch v3

[Brady Eidson]

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.

[WebKit Commit Bot]

Comment on attachment 276566 [details]
New patch v3

Clearing flags on attachment: 276566

Committed r199643: <http://trac.webkit.org/changeset/199643>

[Brady Eidson]

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...  =/

[Darin Adler]

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.

[Brady Eidson]

(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.

[Chris Dumez]

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<>() ?