Bug 33016 – [V8] Use script state of the current isolated world when creating ScriptCallStack

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Yury Semikhatsky]

Currently ScriptCallStack is created based on current Frame's main world context. This is wrong since current context maybe one from isolated world.

[Yury Semikhatsky]

Created attachment 45594 [details]
patch

[Yury Semikhatsky]

Created attachment 45595 [details]
patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45595 [details] without any errors.

[Dimitri Glazkov (Google)]

Comment on attachment 45595 [details]
patch

I like the premise of the patch. I don't like that we're adding extra methods to already humongous V8Proxy and V8IsolatedWorld. ScriptState is supposed to be a thin wrapper, and I certainly don't want us to carry it around in either of these instances. 

Can you rework it perhaps to just add static create methods to ScriptState, and put world-aware logic there?

[Yury Semikhatsky]

(In reply to comment #4)
> (From update of attachment 45595 [details])
> I like the premise of the patch. I don't like that we're adding extra methods
> to already humongous V8Proxy and V8IsolatedWorld. ScriptState is supposed to be
> a thin wrapper, and I certainly don't want us to carry it around in either of
> these instances. 
> 
> Can you rework it perhaps to just add static create methods to ScriptState, and
> put world-aware logic there?
ScriptState unlike other Script* objetcs is not a value type. I don't want to manage ScriptState life time and my point was to cache pointer to ScriptState on the objects that can be casted to ScriptState* This way the ScriptState instance would live as long as the original object stays alive. Otherwise if one needed to store a pointer to ScriptState from the call stack he would have to make a copy of the ScriptCallStack.state() since the original ScriptState object would be destroyed along with stack allocated ScriptCallStack object. I guess original ScriptState design assumes that clients should be able to keep pointer to ScriptState instance while original context exists. Please correct me if I'm wrong.

[Yury Semikhatsky]

Created attachment 45617 [details]
patch

[Adam Barth]

Does this change have an observable change in behavior?  If so, we'll need a
test.

[WebKit Review Bot]

Attachment 45617 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/ScriptCallStack.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1

[Yury Semikhatsky]

(In reply to comment #7)
> Does this change have an observable change in behavior?  If so, we'll need a
> test.

With current implementation of InspectorController the change should not affect observable behavior.

[Yury Semikhatsky]

Created attachment 45618 [details]
patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45618 [details] without any errors.

[Dimitri Glazkov (Google)]

Comment on attachment 45618 [details]
patch

r=me.

Please file a bug regarding the new role of ScriptState.

[Yury Semikhatsky]

(In reply to comment #12)
> Please file a bug regarding the new role of ScriptState.

Done: https://bugs.webkit.org/show_bug.cgi?id=33047

[Yury Semikhatsky]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/v8/ScriptCallStack.cpp
	M	WebCore/bindings/v8/ScriptCallStack.h
	M	WebCore/bindings/v8/ScriptController.cpp
	M	WebCore/bindings/v8/ScriptController.h
	M	WebCore/bindings/v8/V8IsolatedWorld.cpp
	M	WebCore/bindings/v8/V8IsolatedWorld.h
Committed r52653