Bug 102158 – [V8] Merge getCachedWrapper(Node*) into DOMDataStore

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

[V8] Merge getCachedWrapper(Node*) into DOMDataStore

[Adam Barth]

Created attachment 174035 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 174035 [details]
Patch

Seems reasonable.  Normal caveat applies (talk to harken if you feel you need).

[Eric Seidel (no email)]

I mean haraken. :)

[Kentaro Hara]

Comment on attachment 174035 [details]
Patch

LGTM too

[Adam Barth]

Created attachment 175085 [details]
Patch for landing

[Adam Barth]

Created attachment 175086 [details]
Patch for landing

[Adam Barth]

Created attachment 175087 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 175087 [details]
Patch for landing

Rejecting attachment 175087 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
kendImpl.h:32,
                 from Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:27:
Source/WebCore/Modules/indexeddb/IDBBackingStore.h:76: error: ISO C++ forbids declaration of 'DISALLOW_COPY_AND_ASSIGN' with no type
  CXX(target) out/Release/obj.target/webcore_remaining/Source/WebCore/Modules/indexeddb/IDBCursorWithValue.o
make: *** [out/Release/obj.target/webcore_remaining/Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/14894386

[Adam Barth]

I don't understand how this compiles without my patch...  DISALLOW_COPY_AND_ASSIGN isn't a WebKit macro...

[WebKit Review Bot]

Comment on attachment 175087 [details]
Patch for landing

Clearing flags on attachment: 175087

Committed r135230: <http://trac.webkit.org/changeset/135230>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

(In reply to comment #10)
> (From update of attachment 175087 [details])
> Clearing flags on attachment: 175087
> 
> Committed r135230: <http://trac.webkit.org/changeset/135230>

This commit caused compilation failure on Chromium Debug builders: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg%29/builds/1943/steps/compile/logs/stdio

I removed the ASSERT for now in http://trac.webkit.org/changeset/135244.

[Kentaro Hara]

Reopening to attach new patch.

[Kentaro Hara]

Created attachment 175140 [details]
Patch

[Kentaro Hara]

Comment on attachment 175140 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175140&action=review

> Source/WebCore/bindings/v8/DOMDataStore.h:66
> +        if (LIKELY(!DOMWrapperWorld::isolatedWorldsExist())) {

Just a confirmation: !v8::Context::InContext() is not needed here, right?

[Adam Barth]

Comment on attachment 175140 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175140&action=review

>> Source/WebCore/bindings/v8/DOMDataStore.h:66
>> +        if (LIKELY(!DOMWrapperWorld::isolatedWorldsExist())) {
> 
> Just a confirmation: !v8::Context::InContext() is not needed here, right?

Correct.

[Adam Barth]

> This commit caused compilation failure on Chromium Debug builders: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg%29/builds/1943/steps/compile/logs/stdio
> 
> I removed the ASSERT for now in http://trac.webkit.org/changeset/135244.

Thanks.  Yes, now that this function is static, we can't assert something about the object's member variables.

[WebKit Review Bot]

Comment on attachment 175140 [details]
Patch

Clearing flags on attachment: 175140

Committed r135257: <http://trac.webkit.org/changeset/135257>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

This (In reply to comment #18)
> (From update of attachment 175140 [details])
> Clearing flags on attachment: 175140
> 
> Committed r135257: <http://trac.webkit.org/changeset/135257>

This change broke Debug compilation for the same reason as the original patch:

In file included from ../../WebCore/bindings/v8/V8DOMWrapper.h:34:
../../WebCore/bindings/v8/DOMDataStore.h:67:20: error: call to non-static member function without an object argument
            ASSERT(wrapperIsStoredInObject(object));
                   ^~~~~~~~~~~~~~~~~~~~~~~
../../WTF/wtf/Assertions.h:247:8: note: expanded from macro 'ASSERT'
    (!(assertion) ? \
       ^

http://chromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac%20Builder%20%28dbg%29/builds/34123/steps/compile/logs/stdio

Please make sure your changes at least compile before committing them.

[Kentaro Hara]

(In reply to comment #20)
> This change broke Debug compilation for the same reason as the original patch:

I'm sorry, I'll fix asap after confirming the debug build locally.

[Kentaro Hara]

Created attachment 175168 [details]
Patch

[Kentaro Hara]

Comment on attachment 175168 [details]
Patch

I confirmed it works in my local Chromiu/Debug build.

[Adam Barth]

Comment on attachment 175168 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175168&action=review

> Source/WebCore/bindings/v8/DOMDataStore.h:67
> -            ASSERT(wrapperIsStoredInObject(object));
> +            ASSERT(current(isolate)->wrapperIsStoredInObject(object));

This isn't right.  Let's just remove the assertion.

[Kentaro Hara]

(In reply to comment #24)
> > Source/WebCore/bindings/v8/DOMDataStore.h:67
> > -            ASSERT(wrapperIsStoredInObject(object));
> > +            ASSERT(current(isolate)->wrapperIsStoredInObject(object));
> 
> This isn't right.  Let's just remove the assertion.

It's OK to remove the assertion, but why isn't it right?

[Adam Barth]

(In reply to comment #25)
> (In reply to comment #24)
> > > Source/WebCore/bindings/v8/DOMDataStore.h:67
> > > -            ASSERT(wrapperIsStoredInObject(object));
> > > +            ASSERT(current(isolate)->wrapperIsStoredInObject(object));
> > 
> > This isn't right.  Let's just remove the assertion.
> 
> It's OK to remove the assertion, but why isn't it right?

You're right, this assertion is correct.  Node implies that we must be on the main thread, and the non-existence of isolated worlds implies that this must be the main world.

[Kentaro Hara]

Comment on attachment 175168 [details]
Patch

I need to rebase with ToT.

[Kentaro Hara]

Created attachment 175303 [details]
patch for landing

[Adam Barth]

Comment on attachment 175303 [details]
patch for landing

One moment.  This patch is based on something we might need to roll out because of  bug 102852

[Kentaro Hara]

ok, feel free to insert this assertion when you reland your patch.

[Adam Barth]

Comment on attachment 175303 [details]
patch for landing

One moment.

[Adam Barth]

If you don't mind, we can skip this patch because I'm changing how this stuff works a bit in bug 102854.