Bug 102102 – Add DOMRequestState to maintain world/ScriptExecutionContext state

• Component: New Bugs
• Status: RESOLVED FIXED

[Alec Flett]

Add DOMRequestState to maintain world/ScriptExecutionContext state

[Alec Flett]

Created attachment 173919 [details]
Patch

[Alec Flett]

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.

[Adam Barth]

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)"

[Alec Flett]

Created attachment 173940 [details]
Patch

[Adam Barth]

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.

[Alec Flett]

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

[Adam Barth]

> 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.

[Alec Flett]

Created attachment 173951 [details]
Patch for landing

[Alec Flett]

Created attachment 173954 [details]
Patch for landing

[WebKit Review Bot]

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

[Alec Flett]

Created attachment 173975 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 173975 [details]
Patch for landing

Clearing flags on attachment: 173975

Committed r134523: <http://trac.webkit.org/changeset/134523>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 102218

[Kentaro Hara]

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)

[Kentaro Hara]

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

[Alec Flett]

Created attachment 174202 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 174202 [details]
Patch for landing

Clearing flags on attachment: 174202

Committed r134632: <http://trac.webkit.org/changeset/134632>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.