Bug 101573 – [V8] Add context checks to WorldContextHandle and V8DOMWindowShell

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

[Dan Carney]

[V8] Add context checks to WorldContextHandle and V8DOMWindowShell

[Dan Carney]

Created attachment 172976 [details]
Patch

[Adam Barth]

Comment on attachment 172976 [details]
Patch

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

Thanks.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:471
> +    V8DOMWrapper::setJSWrapperForDOMObject(PassRefPtr<DOMWindow>(window), windowWrapper);

The explicit call to PassRefPtr<DOMWindow> shouldn't be necessary here.

[Dan Carney]

(In reply to comment #2)
> The explicit call to PassRefPtr<DOMWindow> shouldn't be necessary here.

I just copied it.  I'll move it. 

It looks like there were some new IDB bugs recently introduced, so this patch is failing 4 idb tests. Should I disable the tests in chrome and file a bug for someone to fix that or wait for fix before committing this?

[WebKit Review Bot]

Comment on attachment 172976 [details]
Patch

Attachment 172976 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14781006

New failing tests:
http/tests/inspector/indexeddb/database-data.html
http/tests/inspector/indexeddb/database-structure.html
http/tests/inspector/indexeddb/database-names.html
http/tests/inspector/indexeddb/resources-panel.html

[Adam Barth]

> It looks like there were some new IDB bugs recently introduced, so this patch is failing 4 idb tests. Should I disable the tests in chrome and file a bug for someone to fix that or wait for fix before committing this?

We should see what jsbell and alecflett would like you to do.

It looks like there are also some LayoutTests that fail with your patch.

[Dan Carney]

(In reply to comment #5)
> > It looks like there were some new IDB bugs recently introduced, so this patch is failing 4 idb tests. Should I disable the tests in chrome and file a bug for someone to fix that or wait for fix before committing this?
> 
> We should see what jsbell and alecflett would like you to do.
> 
> It looks like there are also some LayoutTests that fail with your patch.

I added blocking bug 101725 which describes the problem and offers an ugly potential solution.

[Joshua Bell]

(Apologies, I'm a gardening today so a bit distracted.)

(In reply to comment #3)
> 
> It looks like there were some new IDB bugs recently introduced, so this patch is failing 4 idb tests. Should I disable the tests in chrome and file a bug for someone to fix that or wait for fix before committing this?

The 4 inspector/indexeddb tests that are listed above?

I've cc'd vsevik on this bug.

The inspector's IDB code was just rewritten to talk to the front-end APIs rather than the back-end APIs which may have introduced the problem, given that it will be calling into IDB APIs without necessarily having V8 on the stack.

[Dan Carney]

Created attachment 175399 [details]
Patch

[Dan Carney]

(In reply to comment #8)
> Created an attachment (id=175399) [details]
> Patch

rebased with latest changes.
added CRASH instead of ASSERT when no context is entered and UseCurrentWorld is called

[Adam Barth]

Comment on attachment 172976 [details]
Patch

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

>> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:471
>> +    V8DOMWrapper::setJSWrapperForDOMObject(PassRefPtr<DOMWindow>(window), windowWrapper);
> 
> The explicit call to PassRefPtr<DOMWindow> shouldn't be necessary here.

Looks like this comment still applies to the final patch.

[Dan Carney]

(In reply to comment #10)
> (From update of attachment 172976 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172976&action=review
> 
> >> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:471
> >> +    V8DOMWrapper::setJSWrapperForDOMObject(PassRefPtr<DOMWindow>(window), windowWrapper);
> > 
> > The explicit call to PassRefPtr<DOMWindow> shouldn't be necessary here.
> 
> Looks like this comment still applies to the final patch.

Yeah, I had to rebase that patch by hand and didn't see it.

[WebKit Review Bot]

Comment on attachment 175399 [details]
Patch

Rejecting attachment 175399 [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:
ngs/v8/V8DOMWindowShell.cpp
Hunk #1 FAILED at 80.
Hunk #2 succeeded at 328 (offset -98 lines).
Hunk #3 succeeded at 345 (offset -98 lines).
1 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/bindings/v8/V8DOMWindowShell.cpp.rej
patching file Source/WebCore/bindings/v8/V8DOMWindowShell.h
patching file Source/WebCore/bindings/v8/WorldContextHandle.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

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

[Dan Carney]

(In reply to comment #10)
> (From update of attachment 172976 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172976&action=review
> 
> >> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:471
> >> +    V8DOMWrapper::setJSWrapperForDOMObject(PassRefPtr<DOMWindow>(window), windowWrapper);
> > 
> > The explicit call to PassRefPtr<DOMWindow> shouldn't be necessary here.
> 
> Looks like this comment still applies to the final patch.

taking it away gives a compile error

[Adam Barth]

ok

[Dan Carney]

Created attachment 175407 [details]
Patch

[Dan Carney]

Comment on attachment 175407 [details]
Patch

re-rebased

[WebKit Review Bot]

Comment on attachment 175407 [details]
Patch

Rejecting attachment 175407 [details] from commit-queue.

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

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

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

[Kentaro Hara]

Comment on attachment 175407 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

You need to write Adam Barth here.

[Dan Carney]

Created attachment 175408 [details]
Patch

[WebKit Review Bot]

Comment on attachment 175408 [details]
Patch

Clearing flags on attachment: 175408

Committed r135383: <http://trac.webkit.org/changeset/135383>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 102935

[Dan Carney]

Comment on attachment 175408 [details]
Patch

reason for rollback fixed

[WebKit Review Bot]

Comment on attachment 175408 [details]
Patch

Clearing flags on attachment: 175408

Committed r135433: <http://trac.webkit.org/changeset/135433>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 103029

[Yury Semikhatsky]

(In reply to comment #23)
> (From update of attachment 175408 [details])
> reason for rollback fixed

I don't think it is fixed. The test still fails both on bots an on my local build.

[Dan Carney]

Comment on attachment 175408 [details]
Patch

fixed mac build

[WebKit Review Bot]

Comment on attachment 175408 [details]
Patch

Clearing flags on attachment: 175408

Committed r135513: <http://trac.webkit.org/changeset/135513>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.