Bug 101573 - [V8] Add context checks to WorldContextHandle and V8DOMWindowShell
Summary: [V8] Add context checks to WorldContextHandle and V8DOMWindowShell
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 101725 102935 102941 103029
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-08 03:01 PST by Dan Carney
Modified: 2012-11-22 05:52 PST (History)
9 users (show)

See Also:


Attachments
Patch (5.69 KB, patch)
2012-11-08 03:05 PST, Dan Carney
no flags Details | Formatted Diff | Diff
Patch (5.53 KB, patch)
2012-11-21 03:03 PST, Dan Carney
no flags Details | Formatted Diff | Diff
Patch (5.49 KB, patch)
2012-11-21 03:50 PST, Dan Carney
no flags Details | Formatted Diff | Diff
Patch (5.49 KB, patch)
2012-11-21 03:57 PST, Dan Carney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Carney 2012-11-08 03:01:15 PST
[V8] Add context checks to WorldContextHandle and V8DOMWindowShell
Comment 1 Dan Carney 2012-11-08 03:05:06 PST
Created attachment 172976 [details]
Patch
Comment 2 Adam Barth 2012-11-08 10:20:16 PST
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.
Comment 3 Dan Carney 2012-11-08 11:16:12 PST
(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?
Comment 4 WebKit Review Bot 2012-11-08 16:51:37 PST
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
Comment 5 Adam Barth 2012-11-09 00:41:48 PST
> 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.
Comment 6 Dan Carney 2012-11-09 01:41:24 PST
(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.
Comment 7 Joshua Bell 2012-11-09 09:59:25 PST
(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.
Comment 8 Dan Carney 2012-11-21 03:03:58 PST
Created attachment 175399 [details]
Patch
Comment 9 Dan Carney 2012-11-21 03:06:20 PST
(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
Comment 10 Adam Barth 2012-11-21 03:11:27 PST
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.
Comment 11 Dan Carney 2012-11-21 03:14:49 PST
(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.
Comment 12 WebKit Review Bot 2012-11-21 03:26:24 PST
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
Comment 13 Dan Carney 2012-11-21 03:42:22 PST
(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
Comment 14 Adam Barth 2012-11-21 03:45:10 PST
ok
Comment 15 Dan Carney 2012-11-21 03:50:54 PST
Created attachment 175407 [details]
Patch
Comment 16 Dan Carney 2012-11-21 03:51:43 PST
Comment on attachment 175407 [details]
Patch

re-rebased
Comment 17 WebKit Review Bot 2012-11-21 03:54:30 PST
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
Comment 18 Kentaro Hara 2012-11-21 03:55:10 PST
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.
Comment 19 Dan Carney 2012-11-21 03:57:49 PST
Created attachment 175408 [details]
Patch
Comment 20 WebKit Review Bot 2012-11-21 04:25:05 PST
Comment on attachment 175408 [details]
Patch

Clearing flags on attachment: 175408

Committed r135383: <http://trac.webkit.org/changeset/135383>
Comment 21 WebKit Review Bot 2012-11-21 04:25:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 WebKit Review Bot 2012-11-21 06:38:12 PST
Re-opened since this is blocked by bug 102935
Comment 23 Dan Carney 2012-11-21 08:13:12 PST
Comment on attachment 175408 [details]
Patch

reason for rollback fixed
Comment 24 WebKit Review Bot 2012-11-21 14:24:49 PST
Comment on attachment 175408 [details]
Patch

Clearing flags on attachment: 175408

Committed r135433: <http://trac.webkit.org/changeset/135433>
Comment 25 WebKit Review Bot 2012-11-21 14:24:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 WebKit Review Bot 2012-11-21 23:44:34 PST
Re-opened since this is blocked by bug 103029
Comment 27 Yury Semikhatsky 2012-11-21 23:49:09 PST
(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.
Comment 28 Dan Carney 2012-11-22 05:38:48 PST
Comment on attachment 175408 [details]
Patch

fixed mac build
Comment 29 WebKit Review Bot 2012-11-22 05:52:48 PST
Comment on attachment 175408 [details]
Patch

Clearing flags on attachment: 175408

Committed r135513: <http://trac.webkit.org/changeset/135513>
Comment 30 WebKit Review Bot 2012-11-22 05:52:54 PST
All reviewed patches have been landed.  Closing bug.