Bug 32554 – Create injected script instance per inspected frame context

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

[Yury Semikhatsky]

Currently there is one InjectedScript instance per inspected page which means that it has access to all frames contents. This may lead to security problems. To protect against them WebKit uses quaranined objects, Chromium currently has utility context which basically has the same proviledges as the main frame in the page. Would much better to have each InjectedScript instance access exactly one frame and send all data in serialized form to the frontend by means of InjectedScriptHost.

This should allow inspecting iframes from different domains and should make it possible to get rid of quarantined objects since all the communication with the inspected frame would be serialized:
Inspected Frame <--> InjectedScript -->o-- InjectedScriptHost --> InspectorFrontend
(all the messages between InjectedScript and InjectedScriptHost should be serialized).

[Yury Semikhatsky]

Created attachment 45566 [details]
initial patch

The idea of the change is to compile all JavaScript routines(InjectedScript) required by the Web Inspector directly in the context of the inspected frame. The only interface for the InjectedScript to talk to the inspector is InjectedScriptHost. All the data passed to and received from the InjectedScript are supposed to be serialized. We cannot rely on window.JSON object from the inspected context as the object may have been modified by the inspected script. In the patch I'm using custom JSON serializer but I'm going to employ SerializedScriptValue for serializing the data. Serializing all messages and having separate InjectedScript instance per inspected context allows to get rid of object quarantines.

InjectedScript instance is cached on the inspected context. To avoid cluttering inspected global object, reference to InjectedScript is stored as a property of JSDOMGlobalObject in case of JSC and as a hidden property on the context's global object in case of V8.

There are two major types of operations that require InjectedScript instance:
1) DOM Tree and CSS styles inspection.
2) Inspection of JS objects including evaluation results, objects written to console and call stack variables.
In the first case we can find frame to which inspected DOM node belongs and use InjectedScript from that frame. To handle the latter case each InjectedScript object is assigned unique id. This id is stored as a part of corresponding object proxies so that when inspector frontend needs to access the InjectedScript it can find the InjectedScript instance by that id. So each action that is supposed to be executed by InjectedScript is accompanied with either DOM Node id or InjectedSript id.

There is one persistent reference to InjectedScript JS object from native part of the inspector that is mapping from id to InjectedScript. The mapping is cleared on Page navigation.

This patch should be broken down into several smaller pieces but I think as a whole it gives a better understanding on the change.

[Yury Semikhatsky]

Created attachment 45659 [details]
patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:50:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:52:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:59:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:62:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:67:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:69:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:71:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:73:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:74:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:206:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:204:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 20

[Adam Barth]

Comment on attachment 45659 [details]
patch

Would you be willing to fix the style nits?

[Yury Semikhatsky]

Created attachment 46120 [details]
patch

Fixed style errors.

[Adam Barth]

Comment on attachment 46120 [details]
patch

No ChangeLog.

This code is fantastically complicated but there are no tests!  How will we avoid breaking this in the future?

 82     v8::Handle<v8::Context> utilityContext = scriptState->context();
 83     v8::Context::Scope contextScope(utilityContext);

It's important to use local handles in v8::Context::Scope.  Is scriptState->context() a local handle?  The risk is the persistent handle might be disposed while this context is still on the stack -> disaster.

 1380   var backslashesRe = /\\["\\\/bfnrtu]/g;
 1381   var simpleValuesRe =
 1382       /"[^"\\\n\r\u2028\u2029\x00-\x08\x10-\x1f\x80-\x9f]*"|true|false|null|-?\d+(?:\.\d*)?(?:[eE][+\-]?\d+)?/g;
 1383   var openBracketsRe = /(?:^|:|,)(?:[\s\u2028\u2029]*\[)+/g;
 1384   var remainderRe = /^[\],:{}\s\u2028\u2029]*$/;

Why aren't we using the native JSON parser?

I have no idea whether the rest is correct, but hopefully we have other folks who can review it.  R- for no ChangeLog.

I'm concerned about the lack of tests in this area.

[Adam Barth]

For the DOM and CSS properties, you can just use the isolated worlds mechanism.  That will give you a safe context from which to examine the page.  Of course, you won't be able to see the JS objects.

[Adam Barth]

w.r.t. worrying about the page overwriting window.JSON, doesn't the utility context have a native JSON object?  Why can't we just use that?

This patch is very, very complicated.  Why can't we just use an isolated world with an "unsafeWindow" property that points to the raw global object of the inspected page?  From there you can crawl the JavaScript objects.

[Pavel Feldman]

(In reply to comment #6)

> This code is fantastically complicated but there are no tests!  How will we
> avoid breaking this in the future?

Some inspector tests are using injected scripts. If they continue working fine after the migration from quarantines to the injected scripts, we should be fine from the inspector standpoint. There is also a test on cross-domain style inspection that is currently failing, but should start passing after the patch. We probably should include it into this patch.

It does not look fantastically complicated to me, but I am well aware of the context. What parts confuse you? We should make clear it reads well for everybody.

> Why aren't we using the native JSON parser?

When standard JSON stringifier finds toJSON property on an object, it uses it instead of its own generic implementation. Libraries such as prototype add this property to all the objects out there, hence confusing the stringifier. The goal here is to prevent page from affecting the serialization. Of course, page is always capable of adding stuff to the prototype (i.e. add properties or define getters/setters), but we will sanitize and process only the stuff we understand. (InjectedScript is primarily about dom inspection)

> I have no idea whether the rest is correct, but hopefully we have other folks
> who can review it.  R- for no ChangeLog.

I'll do the inspector-related review. We will need someone to validate the JSC part as well.

(In reply to comment #7)
> For the DOM and CSS properties, you can just use the isolated worlds mechanism.
>  That will give you a safe context from which to examine the page.  Of course,
> you won't be able to see the JS objects.

DOM inspection implies inspection of the node properties, including JS ones.

(In reply to comment #8)
> w.r.t. worrying about the page overwriting window.JSON, doesn't the utility
> context have a native JSON object?  Why can't we just use that?
> 

I think I covered it above. It is not JSON we worry about, it is toJSON.

> This patch is very, very complicated.  Why can't we just use an isolated world
> with an "unsafeWindow" property that points to the raw global object of the
> inspected page?  From there you can crawl the JavaScript objects.

That was the original idea. But then we thought we could avoid duping the number of worlds and managing these worlds lifetime. I think what you are suggesting is not reducing the complexity, but is increasing it. What part of the patch would is remove? ScriptObject-related part we need for not diverging inspector implementation, JSON-related part is needed for pushing information further into the frontend entity.

[Adam Barth]

> It does not look fantastically complicated to me, but I am well aware of the
> context. What parts confuse you? We should make clear it reads well for
> everybody.

There's a lot of direct manipulation of V8 in strange ways.  For example, you seems to be grabbing the global object out of one context and using it as the global object for a script compiled into another context.  More over, you don't actually grab the global object from the first context, you bypass the proxy object to get at the "inner" window.  That's all super subtle stuff.

For example, what do the security checks do when they see a context like this?  They're used to figuring out the DOMWindow from a normally arranged context (e.g., with a WindowProxy object).  What does the other code do that assumes it can find the inner window by looking at the Global()->GetPrototype()? 

You have a bunch of locations where you're grabbing a v8::Handle and sticking it in a V8::Context:Scope.  You need to always using a v8::Local for that purpose otherwise you risk memory corruption if your v8:: Persistent gets Disposed.

At one point in the patch, you've got a weak reference to a V8 object that's involved in manually twiddling the refcounts on a WebCore object.  Does that mean we can enter WebCore destructors during GC?  Do we let that happen elsewhere?

> I'll do the inspector-related review. We will need someone to validate the JSC
> part as well.

If you're confident in reviewing this patch, then that's good enough for me.  This stuff is just complicated.  It's taken a long time to beat the bugs out of the normal way of doing all this stuff, and that gets hammered on by the web and has an entire suite of tests.

> DOM inspection implies inspection of the node properties, including JS ones.

Ok.  You're more familiar with the needs of the inspector than I am.

> That was the original idea. But then we thought we could avoid duping the
> number of worlds and managing these worlds lifetime.

I don't understand what you mean here.  Don't you have to manage the lifetime of the utility context?

> I think what you are
> suggesting is not reducing the complexity, but is increasing it. What part of
> the patch would is remove?

All the magical bits about that create a context that got half another context spliced into it.  All the call sites to compile and execute script.

I don't know that what I'm suggesting is the right approach.  It's just scary to me that we're going to create all these exotic V8 objects and situations.

[Pavel Feldman]

(In reply to comment #10)

> There's a lot of direct manipulation of V8 in strange ways.  For example, you
> seems to be grabbing the global object out of one context and using it as the
> global object for a script compiled into another context. 

There are no additional contexts, it is just the naming and comments that make it confusing.

> +    // Create utility context. At the moment we inject the script directly into the inspected context.
> +    v8::Handle<v8::Context> utilityContext = scriptState->context();

I'll make sure to mention it in review.

[Yury Semikhatsky]

(In reply to comment #6)
> (From update of attachment 46120 [details])
> No ChangeLog.
>
There is no ChangeLog entry since I'm going to break this patch down into a few smaller ones so that they don't break Chromium build. However, I wanted to show the whole change first to get feedback on it.

>  82     v8::Handle<v8::Context> utilityContext = scriptState->context();
>  83     v8::Context::Scope contextScope(utilityContext);
> 
> It's important to use local handles in v8::Context::Scope.  Is
> scriptState->context() a local handle?  The risk is the persistent handle might
> be disposed while this context is still on the stack -> disaster.
>
In general that is true but in this particular case we know that the context will exist while the scriptState exists. In the method body no specific properties of Local handle are used so I preferred to use just Handle. But you are right to make the code more clear I will change Handle to Local where appropriate.


>  1380   var backslashesRe = /\\["\\\/bfnrtu]/g;
>  1381   var simpleValuesRe =
>  1382      
> /"[^"\\\n\r\u2028\u2029\x00-\x08\x10-\x1f\x80-\x9f]*"|true|false|null|-?\d+(?:\.\d*)?(?:[eE][+\-]?\d+)?/g;
>  1383   var openBracketsRe = /(?:^|:|,)(?:[\s\u2028\u2029]*\[)+/g;
>  1384   var remainderRe = /^[\],:{}\s\u2028\u2029]*$/;
> 
> Why aren't we using the native JSON parser?
> 
Pavel already commented this, we need to protect inspectors code against custom toJSON methods and against modified window.JSON object. 


(In reply to comment #8)
> w.r.t. worrying about the page overwriting window.JSON, doesn't the utility
> context have a native JSON object?  Why can't we just use that?
> 
There are no additional contexts any longer. All the code is compiled right in the inspected context. And it uses the same global object. The variables naming is misleading, it remained from the previous Chromium implementation. I'll rename them.


(In reply to comment #10)
> There's a lot of direct manipulation of V8 in strange ways.  For example, you
> seems to be grabbing the global object out of one context and using it as the
> global object for a script compiled into another context.  More over, you don't
> actually grab the global object from the first context, you bypass the proxy
> object to get at the "inner" window.  That's all super subtle stuff.
> 
There is no additional context, utilityContext is just the same instance as the inspected context and the injected script is compiled in that context and with its global object. We bypass the proxy object to store reference to the InjectedScript object on its prototype so that it is collected along with the prototype on Page navigation.

> For example, what do the security checks do when they see a context like this? 
> They're used to figuring out the DOMWindow from a normally arranged context
> (e.g., with a WindowProxy object).  What does the other code do that assumes it
> can find the inner window by looking at the Global()->GetPrototype()? 
>
We use the prototype only for referencing the InjectedScript instance. In the all other places the proxy object is used.

 
> You have a bunch of locations where you're grabbing a v8::Handle and sticking
> it in a V8::Context:Scope.  You need to always using a v8::Local for that
> purpose otherwise you risk memory corruption if your v8:: Persistent gets
> Disposed.
> 
Done.

> At one point in the patch, you've got a weak reference to a V8 object that's
> involved in manually twiddling the refcounts on a WebCore object.  Does that
> mean we can enter WebCore destructors during GC?
Yes, that's correct. 

> Do we let that happen elsewhere?
>
V8 bindings do that. We couldn't reuse that implementation because we had to create V8 wrappers in the utility context while the bindings code always expects the context to be a window context. Now that we inject the script directly into the inspected context it should be possible to create it using the generic v8 bindings facilities.

> > That was the original idea. But then we thought we could avoid duping the
> > number of worlds and managing these worlds lifetime.
> 
> I don't understand what you mean here.  Don't you have to manage the lifetime
> of the utility context?
> 
We need to manage only lifetime of the InjectedScript instance. We store it as a hidden property on the global proxy prototype so that it's automatically collected by GC when the prototype is collected.

[Yury Semikhatsky]

Created attachment 46264 [details]
patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/inspector/InspectorController.cpp:1845:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1

[Yury Semikhatsky]

Created attachment 46265 [details]
patch

Renamed utilityContext to inspectedContext, changed Handle to Local.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/inspector/InspectorController.cpp:1845:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1

[Yury Semikhatsky]

Created attachment 46266 [details]
patch

Fixed style error.

[Yury Semikhatsky]

Created attachment 46368 [details]
patch

[Yury Semikhatsky]

Created attachment 46444 [details]
patch

Replaced goog.json implementation with http://www.json.org/json2.js

[Timothy Hatcher]

Comment on attachment 46444 [details]
patch

We don't need json2.js. We have native JSON in JSC and V8. We should use it.

[Timothy Hatcher]

Is that the only option if we need to ignore toJSON? Maybe we need an extension (argument) to JSON.stringify that ignores it.

[Yury Semikhatsky]

(In reply to comment #21)
> Is that the only option if we need to ignore toJSON? Maybe we need an extension
> (argument) to JSON.stringify that ignores it.

I'd like to switch to SerializedScriptValue instead of JSON when v8 implementation of it is landed so I view this as a temporary measure. Having an argument in JSON.stringify that would allow to make the serializer ignore toJSON methods would solve the problem but it would require adding non-standard argument and also there is still a chance the application has modified JSON.serialize.

[Timothy Hatcher]

OK. Please file a bug about removing json2.js and the desire to use SerializedScriptValue.

[Yury Semikhatsky]

(In reply to comment #23)
> OK. Please file a bug about removing json2.js and the desire to use
> SerializedScriptValue.

Done. Bug 32554.

[Pavel Feldman]

Change was big, I commented on it using http://codereview.chromium.org/542055.

[Yury Semikhatsky]

Created attachment 46578 [details]
patch

[Yury Semikhatsky]

(In reply to comment #25)
> Change was big, I commented on it using http://codereview.chromium.org/542055.

Addressed all the comments.

[Yury Semikhatsky]

Created attachment 46579 [details]
patch

[Pavel Feldman]

Posted review comments at http://codereview.chromium.org/542055:

http://codereview.chromium.org/542055/diff/4001/5013
File inspector/InjectedScriptHost.cpp (right):

http://codereview.chromium.org/542055/diff/4001/5013#newcode183
inspector/InjectedScriptHost.cpp:183: ScriptObject injectedScript =
m_idToInjectedScript.get(injectedScriptId);
check for 0 - it is asynchronous.

http://codereview.chromium.org/542055/diff/4001/5025
File inspector/InspectorBackend.cpp (right):

http://codereview.chromium.org/542055/diff/4001/5025#newcode272
inspector/InspectorBackend.cpp:272: bool injectedScriptIdIsNodeId =
injectedScriptId <= 0;
FIXME?

http://codereview.chromium.org/542055/diff/4001/5014
File inspector/InspectorController.cpp (right):

http://codereview.chromium.org/542055/diff/4001/5014#newcode1867
inspector/InspectorController.cpp:1867: ASSERT(false);
you hit this due to async nature of stuff too.

http://codereview.chromium.org/542055/diff/4001/5011
File inspector/InspectorFrontend.cpp (right):

http://codereview.chromium.org/542055/diff/4001/5011#newcode108
inspector/InspectorFrontend.cpp:108: ScriptFunctionCall
wrapFunction(scriptState, injectedScript, "wrapAndStringifyObject");
I really don't like double-json.

http://codereview.chromium.org/542055/diff/4001/5022
File inspector/InspectorFrontend.h (right):

http://codereview.chromium.org/542055/diff/4001/5022#newcode62
inspector/InspectorFrontend.h:62: ScriptArray newScriptArray();
We should enforce typing here and introduce FrontendArray/Object so that
inspected objects don't get into malicious context.

http://codereview.chromium.org/542055/diff/4001/5032
File inspector/front-end/ConsoleView.js (right):

http://codereview.chromium.org/542055/diff/4001/5032#newcode540
inspector/front-end/ConsoleView.js:540:
InjectedScriptAccess.pushNodeToFrontend(object.injectedScriptId, object,
printNode);
InjectedScriptAccess has too many parameters in calls already. I think we
should
use following notation instead:

InjectedScriptAccess.get(object.injectedScriptId).pushNodeToFrontend(object,
printNode); That way you don't force clients to have injected script id as the
first argument of the method.

http://codereview.chromium.org/542055/diff/4001/5039
File inspector/front-end/Database.js (right):

http://codereview.chromium.org/542055/diff/4001/5039#newcode98
inspector/front-end/Database.js:98: InjectedScriptAccess.executeSql(0 /* For
now
use main frame injected script. */, this._id, query, callback);
FIXME?

http://codereview.chromium.org/542055/diff/4001/5038
File inspector/front-end/ElementsPanel.js (right):

http://codereview.chromium.org/542055/diff/4001/5038#newcode239
inspector/front-end/ElementsPanel.js:239: InjectedScriptAccess.nodeByPath(0 /*
main frame injected script id */, this._selectedPathOnReset,
selectLastSelectedNode.bind(this));
InjectedScriptAccess.getDefault() here and below.

http://codereview.chromium.org/542055/diff/4001/5020
File inspector/front-end/InjectedScript.js (right):

http://codereview.chromium.org/542055/diff/4001/5020#newcode913
inspector/front-end/InjectedScript.js:913: return "";
it was a convention - false means "i failed".

http://codereview.chromium.org/542055/diff/4001/5021
File inspector/front-end/InjectedScriptAccess.js (right):

http://codereview.chromium.org/542055/diff/4001/5021#newcode52
inspector/front-end/InjectedScriptAccess.js:52: WebInspector.log(methodName + '
' + injectedScriptId + ' ' + (typeof injectedScriptId));
remove log

http://codereview.chromium.org/542055/diff/4001/5019
File inspector/front-end/inspector.js (right):

http://codereview.chromium.org/542055/diff/4001/5019#newcode396
inspector/front-end/inspector.js:396:
InspectorBackend.setInjectedScriptSource(injectedScriptSource.join(''));
do not use single quotes.
"(" + injectedScriptConstructor + ")"

http://codereview.chromium.org/542055/diff/4001/5019#newcode1156
inspector/front-end/inspector.js:1156: callFrames = JSON.parse(callFrames);
Don't like this either.

http://codereview.chromium.org/542055/diff/4001/5019#newcode1225
inspector/front-end/inspector.js:1225:
parsedArguments.push(JSON.parse(arguments[i]));
Should not need this - FrontendObject in WebKit, dispatch via parse in Chromium
should make us safe.

[Yury Semikhatsky]

Created attachment 46801 [details]
patch

Addressed the comments.

http://codereview.chromium.org/542055/diff/4001/5013
File inspector/InjectedScriptHost.cpp (right):

http://codereview.chromium.org/542055/diff/4001/5013#newcode183
inspector/InjectedScriptHost.cpp:183: ScriptObject injectedScript =
m_idToInjectedScript.get(injectedScriptId);
On 2010/01/14 18:59:03, pfeldman wrote:
> check for 0 - it is asynchronous.

Done.

http://codereview.chromium.org/542055/diff/4001/5025
File inspector/InspectorBackend.cpp (right):

http://codereview.chromium.org/542055/diff/4001/5025#newcode272
inspector/InspectorBackend.cpp:272: bool injectedScriptIdIsNodeId =
injectedScriptId <= 0;
On 2010/01/14 18:59:03, pfeldman wrote:
> FIXME?

Done.

http://codereview.chromium.org/542055/diff/4001/5014
File inspector/InspectorController.cpp (right):

http://codereview.chromium.org/542055/diff/4001/5014#newcode1867
inspector/InspectorController.cpp:1867: ASSERT(false);
On 2010/01/14 18:59:03, pfeldman wrote:
> you hit this due to async nature of stuff too.

Done.

http://codereview.chromium.org/542055/diff/4001/5032
File inspector/front-end/ConsoleView.js (right):

http://codereview.chromium.org/542055/diff/4001/5032#newcode540
inspector/front-end/ConsoleView.js:540:
InjectedScriptAccess.pushNodeToFrontend(object.injectedScriptId, object,
printNode);
On 2010/01/14 18:59:03, pfeldman wrote:
> InjectedScriptAccess has too many parameters in calls already. I think we
should
> use following notation instead:
> 
> InjectedScriptAccess.get(object.injectedScriptId).pushNodeToFrontend(object,
> printNode); That way you don't force clients to have injected script id as the
> first argument of the method.

Done.

http://codereview.chromium.org/542055/diff/4001/5039
File inspector/front-end/Database.js (right):

http://codereview.chromium.org/542055/diff/4001/5039#newcode98
inspector/front-end/Database.js:98: InjectedScriptAccess.executeSql(0 /* For now
use main frame injected script. */, this._id, query, callback);
On 2010/01/14 18:59:03, pfeldman wrote:
> FIXME?

Done.

http://codereview.chromium.org/542055/diff/4001/5038
File inspector/front-end/ElementsPanel.js (right):

http://codereview.chromium.org/542055/diff/4001/5038#newcode239
inspector/front-end/ElementsPanel.js:239: InjectedScriptAccess.nodeByPath(0 /*
main frame injected script id */, this._selectedPathOnReset,
selectLastSelectedNode.bind(this));
On 2010/01/14 18:59:03, pfeldman wrote:
> InjectedScriptAccess.getDefault() here and below.

Done.

http://codereview.chromium.org/542055/diff/4001/5020
File inspector/front-end/InjectedScript.js (right):

http://codereview.chromium.org/542055/diff/4001/5020#newcode913
inspector/front-end/InjectedScript.js:913: return "";
On 2010/01/14 18:59:03, pfeldman wrote:
> it was a convention - false means "i failed".

Reverted. Would be nice to document this unobvious convention, otherwise it
looks like a bug.

http://codereview.chromium.org/542055/diff/4001/5021
File inspector/front-end/InjectedScriptAccess.js (right):

http://codereview.chromium.org/542055/diff/4001/5021#newcode52
inspector/front-end/InjectedScriptAccess.js:52: WebInspector.log(methodName + '
' + injectedScriptId + ' ' + (typeof injectedScriptId));
On 2010/01/14 18:59:03, pfeldman wrote:
> remove log

Done.

http://codereview.chromium.org/542055/diff/4001/5019
File inspector/front-end/inspector.js (right):

http://codereview.chromium.org/542055/diff/4001/5019#newcode396
inspector/front-end/inspector.js:396:
InspectorBackend.setInjectedScriptSource(injectedScriptSource.join(''));
On 2010/01/14 18:59:03, pfeldman wrote:
> do not use single quotes.
> "(" + injectedScriptConstructor + ")"

Done.

http://codereview.chromium.org/542055/diff/4001/5019#newcode1156
inspector/front-end/inspector.js:1156: callFrames = JSON.parse(callFrames);
On 2010/01/14 18:59:03, pfeldman wrote:
> Don't like this either.

This will disappear once we switch to SerializedScriptValue.

[Yury Semikhatsky]

Created attachment 46806 [details]
patch

[Pavel Feldman]

Comment on attachment 46806 [details]
patch

This looks right, but the change is very large. Please test it well. (Styles, databases, local stores, etc.).

[Yury Semikhatsky]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/inspector/styles-iframe.html
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/JSInjectedScriptHostCustom.cpp
	M	WebCore/bindings/js/ScriptController.cpp
	M	WebCore/bindings/js/ScriptController.h
	M	WebCore/bindings/js/ScriptObject.h
	M	WebCore/bindings/js/ScriptValue.cpp
	M	WebCore/bindings/js/ScriptValue.h
	M	WebCore/bindings/v8/ScriptObject.h
	M	WebCore/bindings/v8/ScriptValue.h
	M	WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp
	M	WebCore/inspector/ConsoleMessage.cpp
	M	WebCore/inspector/ConsoleMessage.h
	M	WebCore/inspector/InjectedScriptHost.cpp
	M	WebCore/inspector/InjectedScriptHost.h
	M	WebCore/inspector/InjectedScriptHost.idl
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorBackend.h
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/front-end/AuditsPanel.js
	M	WebCore/inspector/front-end/ConsoleView.js
	M	WebCore/inspector/front-end/DOMAgent.js
	M	WebCore/inspector/front-end/Database.js
	M	WebCore/inspector/front-end/ElementsPanel.js
	M	WebCore/inspector/front-end/ElementsTreeOutline.js
	M	WebCore/inspector/front-end/EventListenersSidebarPane.js
	M	WebCore/inspector/front-end/InjectedScript.js
	M	WebCore/inspector/front-end/InjectedScriptAccess.js
	M	WebCore/inspector/front-end/MetricsSidebarPane.js
	M	WebCore/inspector/front-end/ObjectPropertiesSection.js
	M	WebCore/inspector/front-end/ObjectProxy.js
	M	WebCore/inspector/front-end/PropertiesSidebarPane.js
	M	WebCore/inspector/front-end/ResourcesPanel.js
	M	WebCore/inspector/front-end/ScriptsPanel.js
	M	WebCore/inspector/front-end/StylesSidebarPane.js
	M	WebCore/inspector/front-end/WatchExpressionsSidebarPane.js
	M	WebCore/inspector/front-end/inspector.js
Committed r53467

[Yury Semikhatsky]

Reverted in r53469. Browser crashes if there are already some console messages when opening Web Inspector.

[Yury Semikhatsky]

Created attachment 47038 [details]
patch

Fixed the issue with console messages. Added unit test.

[Yury Semikhatsky]

Created attachment 47039 [details]
patch

[Yury Semikhatsky]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M	LayoutTests/ChangeLog
	A	LayoutTests/inspector/console-log-before-inspector-open-expected.txt
	A	LayoutTests/inspector/console-log-before-inspector-open.html
	M	LayoutTests/inspector/styles-iframe.html
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/JSInjectedScriptHostCustom.cpp
	M	WebCore/bindings/js/ScriptCallStack.h
	M	WebCore/bindings/js/ScriptController.cpp
	M	WebCore/bindings/js/ScriptController.h
	M	WebCore/bindings/js/ScriptObject.h
	M	WebCore/bindings/js/ScriptValue.cpp
	M	WebCore/bindings/js/ScriptValue.h
	M	WebCore/bindings/v8/ScriptObject.h
	M	WebCore/bindings/v8/ScriptValue.h
	M	WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp
	M	WebCore/inspector/ConsoleMessage.cpp
	M	WebCore/inspector/ConsoleMessage.h
	M	WebCore/inspector/InjectedScriptHost.cpp
	M	WebCore/inspector/InjectedScriptHost.h
	M	WebCore/inspector/InjectedScriptHost.idl
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorBackend.h
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/front-end/AuditsPanel.js
	M	WebCore/inspector/front-end/ConsoleView.js
	M	WebCore/inspector/front-end/DOMAgent.js
	M	WebCore/inspector/front-end/Database.js
	M	WebCore/inspector/front-end/ElementsPanel.js
	M	WebCore/inspector/front-end/ElementsTreeOutline.js
	M	WebCore/inspector/front-end/EventListenersSidebarPane.js
	M	WebCore/inspector/front-end/InjectedScript.js
	M	WebCore/inspector/front-end/InjectedScriptAccess.js
	M	WebCore/inspector/front-end/MetricsSidebarPane.js
	M	WebCore/inspector/front-end/ObjectPropertiesSection.js
	M	WebCore/inspector/front-end/ObjectProxy.js
	M	WebCore/inspector/front-end/PropertiesSidebarPane.js
	M	WebCore/inspector/front-end/ResourcesPanel.js
	M	WebCore/inspector/front-end/ScriptsPanel.js
	M	WebCore/inspector/front-end/StylesSidebarPane.js
	M	WebCore/inspector/front-end/WatchExpressionsSidebarPane.js
	M	WebCore/inspector/front-end/inspector.js
Committed r53552

[David Levin]

Comment on attachment 47039 [details]
patch

Patch rolled out due to lots of fast/profile layout test failures.

[David Levin]

Rolled out change with http://trac.webkit.org/changeset/53558

[Yury Semikhatsky]

Created attachment 47058 [details]
patch

Make profiler tests pass.

[Yury Semikhatsky]

Created attachment 47059 [details]
Diff between the two latest patches

[Yury Semikhatsky]

Created attachment 47123 [details]
patch

This is a part of the big patch that I'd like to commit first so that we can prepare Chromium for the new code.

[Yury Semikhatsky]

Created attachment 47144 [details]
patch

This patch is needed to prepare Chromium for the changes in the signature of InspectorBackend::dispatchOnInjectedScript before the bigger patch is committed, otherwise chromium build will break.

[Eric Seidel (no email)]

Attachment 47144 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/204238

[Yury Semikhatsky]

Created attachment 47146 [details]
patch

Fixed warnings on Mac.

[Pavel Feldman]

Comment on attachment 47146 [details]
patch

Looks good provided that it is a temporary measure. Please add a FIXME and follow up with the migration shortly.

[Yury Semikhatsky]

(In reply to comment #46)
> (From update of attachment 47146 [details])
> Looks good provided that it is a temporary measure. Please add a FIXME and
> follow up with the migration shortly.
Done.

[Yury Semikhatsky]

Comment on attachment 47146 [details]
patch

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/InspectorBackend.h
Committed r53689

Clearing flags.

[Yury Semikhatsky]

Created attachment 47192 [details]
patch

[Yury Semikhatsky]

Landed in r53766.