Bug 102102 - Add DOMRequestState to maintain world/ScriptExecutionContext state
Summary: Add DOMRequestState to maintain world/ScriptExecutionContext state
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: Alec Flett
URL:
Keywords:
Depends on: 102218
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-13 10:08 PST by Alec Flett
Modified: 2012-11-14 11:17 PST (History)
6 users (show)

See Also:


Attachments
Patch (9.29 KB, patch)
2012-11-13 10:10 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (14.77 KB, patch)
2012-11-13 11:55 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch for landing (15.43 KB, patch)
2012-11-13 12:15 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch for landing (15.45 KB, patch)
2012-11-13 12:17 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch for landing (15.49 KB, patch)
2012-11-13 13:38 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch for landing (15.13 KB, patch)
2012-11-14 10:50 PST, Alec Flett
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alec Flett 2012-11-13 10:08:40 PST
Add DOMRequestState to maintain world/ScriptExecutionContext state
Comment 1 Alec Flett 2012-11-13 10:10:06 PST
Created attachment 173919 [details]
Patch
Comment 2 Alec Flett 2012-11-13 10:12:50 PST
abarth@ - finally putting this in a bug. Things that are missing: 
1) tests - I'm not sure how to test this but if you can point me in the right direction, I'll see what I can put together
2) JSC implementation. I'm going to take a crack at this but I welcome any pointers there as well.

adamk@ - this is the patch we talked about monday, it's usable for your private build as-is, and you can use the IDB code here as a reference - the key is that the IDBRequest is created with JS on the stack, and then IDBRequest hangs out until an event is generated/dispatched without JS on the stack.
Comment 3 Adam Barth 2012-11-13 11:16:15 PST
Comment on attachment 173919 [details]
Patch

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

> Source/WebCore/bindings/v8/DOMRequestState.h:39
> +    DOMRequestState(ScriptExecutionContext *scriptExecutionContext)

explicit

"ScriptExecutionContext *" -> "ScriptExecutionContext* "

> Source/WebCore/bindings/v8/DOMRequestState.h:47
> +        Scope(v8::Handle<v8::Context> context)

explicit

> Source/WebCore/bindings/v8/DOMRequestState.h:48
> +            : m_contextScope(context) { }

The { and } each go on their own line.

> Source/WebCore/bindings/v8/DOMRequestState.h:53
> +    const Scope scope()

Woah, I don't know what happens if we try to copy a v8::Context::Scope....

> Source/WebCore/bindings/v8/DOMRequestState.h:55
> +        v8::HandleScope handleScope;    /* do we need this for Local<> ? */

This is bad new bears.  You've got a local handle which will die once this object goes out of scope.

> Source/WebCore/bindings/v8/DOMRequestState.h:59
> +        return Scope(context);

Ok.  So the way to do this is to have the DOMRequestState::Scope object create the HandleScope in its constructor.  The caller looks like:

DOMRequestState::Scope scope(m_requestState)

The DOMRequestState::Scope class then is something like the following:

class Scope {
public:
    explicit Scope(DOMRequestState& state)
        : m_scope(static.context())
    {
    }
private:
    v8::HandleScope m_handleScope;
    v8::Context::Scope m_scope
};

Notice that the HandleScope member precedes the v8::Context::Scope member so that it will be constructed first and destructed last.

DOMRequestState::context is then just "return toV8Context(m_scriptExecutionContext, m_worldContextHandle)"
Comment 4 Alec Flett 2012-11-13 11:55:16 PST
Created attachment 173940 [details]
Patch
Comment 5 Adam Barth 2012-11-13 11:59:11 PST
Comment on attachment 173940 [details]
Patch

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

> Source/WebCore/bindings/v8/DOMRequestState.h:48
> +            : m_contextScope(state.context()) { }

The { and } should each be on their own line.

> Source/WebCore/bindings/v8/DOMRequestState.h:61
> +    ScriptExecutionContext* m_scriptExecutionContext;

How does this pointer get cleared when the ScriptExecutionContext is destroyed?  Maybe we should clear it explicitly in IDBRequest::stop.
Comment 6 Alec Flett 2012-11-13 12:00:23 PST
thanks for the suggestions. My only open issue at this point is if/where to attach context() - you can see I'm now passing DOMRequest* into my IDBBindingUtilities - I don't need it there, but I'm kind of assuming I will on the JSC side? And then I wonder if really I should be passing the DOMRequest::Scope* in there, and hanging context() off of scope? But I don't know if that violates conventions around scope-like objects, who usually just exist on the stack.

I'm also wondering if I can narrow the scope in a few places
Comment 7 Adam Barth 2012-11-13 12:14:07 PST
> I don't need it there, but I'm kind of assuming I will on the JSC side?

We can always add it later if we find we need it then.
Comment 8 Alec Flett 2012-11-13 12:15:04 PST
Created attachment 173951 [details]
Patch for landing
Comment 9 Alec Flett 2012-11-13 12:17:56 PST
Created attachment 173954 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-11-13 13:34:17 PST
Comment on attachment 173954 [details]
Patch for landing

Rejecting attachment 173954 [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:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 12869 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
50>At revision 12869.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/14809977
Comment 11 Alec Flett 2012-11-13 13:38:40 PST
Created attachment 173975 [details]
Patch for landing
Comment 12 WebKit Review Bot 2012-11-13 19:42:23 PST
Comment on attachment 173975 [details]
Patch for landing

Clearing flags on attachment: 173975

Committed r134523: <http://trac.webkit.org/changeset/134523>
Comment 13 WebKit Review Bot 2012-11-13 19:42:27 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 WebKit Review Bot 2012-11-14 05:39:34 PST
Re-opened since this is blocked by bug 102218
Comment 15 Kentaro Hara 2012-11-14 06:50:48 PST
This patch breaks Chromium windows build. The build succeeds in ninja but fails in MSVS (Sorry I'm now at home and cannot find the log). This would indicate that something is wrong with build files. See a comment from Jochen here (https://bugs.webkit.org/show_bug.cgi?id=102218#c2)
Comment 16 Kentaro Hara 2012-11-14 07:07:00 PST
Here is a build log of the windows build:
http://build.chromium.org/p/chromium.win/builders/Win%20Builder/builds/3509/steps/runhooks/logs/stdio
Comment 17 Alec Flett 2012-11-14 10:50:35 PST
Created attachment 174202 [details]
Patch for landing
Comment 18 WebKit Review Bot 2012-11-14 11:17:09 PST
Comment on attachment 174202 [details]
Patch for landing

Clearing flags on attachment: 174202

Committed r134632: <http://trac.webkit.org/changeset/134632>
Comment 19 WebKit Review Bot 2012-11-14 11:17:14 PST
All reviewed patches have been landed.  Closing bug.