| Summary: | [V8] Add context checks to WorldContextHandle and V8DOMWindowShell | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dan Carney <dcarney> | ||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, alecflett, dglazkov, haraken, japhet, jsbell, vsevik, webkit.review.bot, yurys | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | 101725, 102935, 102941, 103029 | ||||||||||||
| Bug Blocks: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Dan Carney
2012-11-08 03:01:15 PST
Created attachment 172976 [details]
Patch
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. (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 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 > 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.
(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. (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. Created attachment 175399 [details]
Patch
(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 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. (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 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 (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 ok Created attachment 175407 [details]
Patch
Comment on attachment 175407 [details]
Patch
re-rebased
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 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. Created attachment 175408 [details]
Patch
Comment on attachment 175408 [details] Patch Clearing flags on attachment: 175408 Committed r135383: <http://trac.webkit.org/changeset/135383> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 102935 Comment on attachment 175408 [details]
Patch
reason for rollback fixed
Comment on attachment 175408 [details] Patch Clearing flags on attachment: 175408 Committed r135433: <http://trac.webkit.org/changeset/135433> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 103029 (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 on attachment 175408 [details]
Patch
fixed mac build
Comment on attachment 175408 [details] Patch Clearing flags on attachment: 175408 Committed r135513: <http://trac.webkit.org/changeset/135513> All reviewed patches have been landed. Closing bug. |