Bug 27703

Summary:

[V8] Teach ScheduledAction::execute about isolated worlds

Product:

WebKit

Reporter:

Adam Barth <abarth>

Component:

WebCore JavaScript

Assignee:

Adam Barth <abarth>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aa, commit-queue, dglazkov, fishd, tkent

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

27533, 27701

Bug Blocks:

27826

Attachments:

Description	Flags
Work-in-progress	none
Patch v1	none
Patch v1	none
Rename queueScriptInIsolatedWorld	none
Scheduled action	none
Patch v1	none
Patch v1	none

Adam Barth

Reported 2009-07-27 03:07:20 PDT

I've finished my current audit of the bindings for isolated world issues. The last thing I found in this class is ScheduledAction::execute, which needs to know how to call back its m_function in the proper world.

Attachments
Work-in-progress (7.31 KB, patch) 2009-07-29 02:31 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch v1 (12.24 KB, patch) 2009-08-02 23:28 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch v1 (24.46 KB, patch) 2009-08-03 11:50 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Rename queueScriptInIsolatedWorld (13.12 KB, patch) 2009-08-04 20:14 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Scheduled action (12.37 KB, patch) 2009-08-04 20:14 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch v1 (10.34 KB, patch) 2009-09-11 01:11 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch v1 (4.43 KB, patch) 2009-09-25 00:59 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Adam Barth

Comment 1 2009-07-27 03:09:21 PDT

We want OwnHandle from 27533 and the new V8Proxy::context(Frame*) from 27701 before implementing this fix. The fix looks really.

Aaron Boodman

Comment 2 2009-07-28 11:27:42 PDT

Note: I would prefer that setTimeout(<string>) from an isolated world executes in the isolated world. My view on the situation is: - eval(), setTimeout(<string>): execute in isolated world - append script tag, location.href = "javascript:": execute in main world

Adam Barth

Comment 3 2009-07-29 02:31:05 PDT

Created attachment 33705 [details] Work-in-progress

Adam Barth

Comment 4 2009-08-02 23:28:58 PDT

Created attachment 33959 [details] Patch v1

Adam Barth

Comment 5 2009-08-02 23:30:03 PDT

Sorry this took so long. I ran into a nasty test_shell bug that confused me for a long time. We'll need to land http://codereview.chromium.org/159782 before these tests will pass downstream.

Adam Barth

Comment 6 2009-08-03 00:13:57 PDT

Comment on attachment 33959 [details] Patch v1 The test_shell change downstream is wrong. I'm not sure how to get the testing harness to run the tests properly.... :(

Adam Barth

Comment 7 2009-08-03 11:50:56 PDT

Created attachment 33993 [details] Patch v1

Adam Barth

Comment 8 2009-08-03 11:56:05 PDT

The new plan is to change test_shell to evaluate the isolated script immediately instead of using the work_queue_. That simplifies life dramatically. I've changed the patch to rename "queueScriptInIsolatedWorld" to "evaluateScriptInIsolateWorld" to indicate that this call is now synchronous.

Eric Seidel (no email)

Comment 9 2009-08-04 19:45:44 PDT

Comment on attachment 33993 [details] Patch v1 These are two separate changes. 1. to change queue to evaluate. 2. to fix ScheduledAction. I don't really know what this does, but I trust you do: 51 m_context.makeWeak(); These changes look both fine, but should be split into two.

Adam Barth

Comment 10 2009-08-04 20:14:49 PDT

Created attachment 34113 [details] Rename queueScriptInIsolatedWorld --- 18 files changed, 47 insertions(+), 17 deletions(-)

Adam Barth

Comment 11 2009-08-04 20:14:54 PDT

Created attachment 34114 [details] Scheduled action --- 10 files changed, 129 insertions(+), 18 deletions(-)

Eric Seidel (no email)

Comment 12 2009-08-04 21:20:33 PDT

Comment on attachment 34113 [details] Rename queueScriptInIsolatedWorld LGTM. I assume this has a corresponding change in Chromium's test_shell?

Eric Seidel (no email)

Comment 13 2009-08-04 21:21:50 PDT

Comment on attachment 34114 [details] Scheduled action LGTM.

Adam Barth

Comment 14 2009-08-04 23:16:52 PDT

Assigned for landing. Landing this is a tricky dance.

Adam Barth

Comment 15 2009-08-04 23:57:38 PDT

Comment on attachment 34113 [details] Rename queueScriptInIsolatedWorld Landed the first patch in http://trac.webkit.org/changeset/46794

Adam Barth

Comment 16 2009-08-06 01:18:19 PDT

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

Adam Barth

Comment 17 2009-09-11 00:26:31 PDT

I had to back this out a while ago.

Adam Barth

Comment 18 2009-09-11 01:11:32 PDT

Created attachment 39414 [details] Patch v1

Eric Seidel (no email)

Comment 19 2009-09-11 16:55:43 PDT

Comment on attachment 39414 [details] Patch v1 I didn't know this until recently, but "explicit" is only needed for single-argument constructors: 50 explicit ScheduledAction(v8::Handle<v8::Context> context, const WebCore::String& code, const KURL& url = KURL())

Adam Barth

Comment 20 2009-09-11 17:01:29 PDT

(In reply to comment #19) > I didn't know this until recently, but "explicit" is only needed for > single-argument constructors: Yeah, I actually removed the keyword in an earlier iteration of this patch, but must have lost that diff at some point. I can remove it on landing.

Dimitri Glazkov (Google)

Comment 21 2009-09-16 10:54:40 PDT

Comment on attachment 39414 [details] Patch v1 r=me, with nits. > +function done() { Brace on new line here and on. > + if (window.layoutTestController) > + layoutTestController.notifyDone(); 4 space indent here and on.

Adam Barth

Comment 22 2009-09-16 11:04:08 PDT

Comment on attachment 39414 [details] Patch v1 Marking cq- so I can fix the nits. Thanks for the review.

Adam Barth

Comment 23 2009-09-16 23:25:25 PDT

Committed r48455: <http://trac.webkit.org/changeset/48455>

Dimitri Glazkov (Google)

Comment 24 2009-09-17 12:16:29 PDT

Patch partially rolled out in http://trac.webkit.org/changeset/48483, after we started seeing crashes in V8Proxy::retrieveFrameForEnteredContext(), like these: chrome_23a0000!v8::internal::Context::global_context+0x3f [c:\b\slave\chromium-rel-xp\build\src\v8\src\contexts.cc @ 59] chrome_23a0000!v8::internal::Context::global_proxy+0x5 [c:\b\slave\chromium-rel-xp\build\src\v8\src\contexts.cc @ 66] chrome_23a0000!v8::Context::Global+0x52 [c:\b\slave\chromium-rel-xp\build\src\v8\src\api.cc @ 2746] chrome_23a0000!WebCore::V8Proxy::retrieveWindow+0x13 [c:\b\slave\chromium-rel-xp\build\src\third_party\webkit\webcore\bindings\v8\v8proxy.cpp @ 504] chrome_23a0000!WebCore::V8Proxy::retrieveFrameForEnteredContext+0x22 [c:\b\slave\chromium-rel-xp\build\src\third_party\webkit\webcore\bindings\v8\v8proxy.cpp @ 526] chrome_23a0000!WebCore::ScriptController::processingUserGesture+0xe [c:\b\slave\chromium-rel-xp\build\src\third_party\webkit\webcore\bindings\v8\scriptcontroller.cpp @ 138] chrome_23a0000!WebCore::FrameLoader::requestFrame+0xf6 [c:\b\slave\chromium-rel-xp\build\src\third_party\webkit\webcore\loader\frameloader.cpp @ 432] chrome_23a0000!WebCore::HTMLFrameElementBase::openURL+0x8f [c:\b\slave\chromium-rel-xp\build\src\third_party\webkit\webcore\html\htmlframeelementbase.cpp @ 106] chrome_23a0000!WebCore::HTMLFrameElementBase::setLocation+0xab [c:\b\slave\chromium-rel-xp\build\src\third_party\webkit\webcore\html\htmlframeelementbase.cpp @ 212]

Adam Barth

Comment 25 2009-09-17 13:22:07 PDT

Is there some way I can reproduce the crashes locally? The LayoutTests seem to run fine on the buildbot...

Dimitri Glazkov (Google)

Comment 26 2009-09-17 13:35:24 PDT

Adam Barth

Comment 27 2009-09-17 13:45:14 PDT

Awesome. Thanks. I'll investigate this weekend.

Adam Barth

Comment 28 2009-09-21 21:58:12 PDT

Comment on attachment 39414 [details] Patch v1 Clearing flags while investigating the crash.

Adam Barth

Comment 29 2009-09-25 00:35:16 PDT

Reduction: <script> function log(msg) { var elmt = document.createElement("div"); elmt.textContent = msg; document.body.appendChild(msg); } var timer = setInterval(function() { clearInterval(timer); log("fired"); }, 10); </script> Maybe it has to do with the clearInterval making us delete something we actually need.

Adam Barth

Comment 30 2009-09-25 00:59:43 PDT

Created attachment 40101 [details] Patch v1

Adam Barth

Comment 31 2009-09-25 01:01:42 PDT

Dimitri, you should be excited to review this patch because it fixes two LayoutTests!

Dimitri Glazkov (Google)

Comment 32 2009-09-25 07:31:34 PDT

Comment on attachment 40101 [details] Patch v1 If I was any more excited, I would burst into confetti-sized pieces, District 9-style. r=me.

WebKit Commit Bot

Comment 33 2009-09-25 09:37:04 PDT

Comment on attachment 40101 [details] Patch v1 Clearing flags on attachment: 40101 Committed r48759: <http://trac.webkit.org/changeset/48759>

WebKit Commit Bot

Comment 34 2009-09-25 09:37:11 PDT

All reviewed patches have been landed. Closing bug.

Kent Tamura

Comment 35 2010-01-13 00:29:33 PST

Comment on attachment 40101 [details] Patch v1 > +++ b/LayoutTests/fast/dom/timer-clear-interval-in-handler-and-generate-error.html > +function log(msg) { > + var elmt = document.createElement("div"); > + elmt.textContent = msg; > + document.body.appendChild(msg); Should this be "document.body.appendChild(elmt);"?

Adam Barth

Comment 36 2010-01-13 00:39:47 PST

> Should this be "document.body.appendChild(elmt);"? Probably. Would you like to create a new bug and post a patch for this?

Kent Tamura

Comment 37 2010-01-13 21:32:38 PST

(In reply to comment #36) > > Should this be "document.body.appendChild(elmt);"? > > Probably. Would you like to create a new bug and post a patch for this? Made Bug#33649

Note You need to log in before you can comment on or make changes to this bug.