Bug 33469 – Support injection of inspector scripts into the inspected ScriptState

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

[Yury Semikhatsky]

Part of Web Inspector is implemented in JavaScript and needs direct access to the inspected JS objects. To facilitate this there should be a way to compile the inspector utilities(called injected script) in the global scope of the inspected ScriptState and somehow store a reference to it(we need to keep the reference since the injected script keeps some state related to the inspected DOM, CSS styles and JS objects).

[Yury Semikhatsky]

Created attachment 46277 [details]
patch

[Yury Semikhatsky]

(In reply to comment #1)
> Created an attachment (id=46277) [details]
> patch

This is a fix for JavaScriptCore. Timothy, could you look at it or forward it to a person who can review the JSC specific code?

[Timothy Hatcher]

Gavin, Geoff or Oliver should review this.

[Geoffrey Garen]

Comment on attachment 46277 [details]
patch

Usually, we don't allow direct interaction between a web page and the inspector, because direct interaction opens a security hole. Why should we make an exception here? Is it safe to do so?

+    void discardInjectedScripts();

This function is never called. Will m_idToInjectedScript grow unbounded?

+    m_nextInjectedScriptId++;

Eventually, this identifier will wrap around, and may collide with previously vended identifiers. To fix this, I would recommend using the (intptr_t) value of injectedScriptObject as your identifier.

[Yury Semikhatsky]

(In reply to comment #4)
> (From update of attachment 46277 [details])
> Usually, we don't allow direct interaction between a web page and the
> inspector, because direct interaction opens a security hole. Why should we make
> an exception here? Is it safe to do so?
> 
Currently for security reasons Web Inspector works with quarantined objects instead of the objects from the inspected script. This patch is a part of a bigger change(bug 32554) that will allow to get rid of object quarantine. It is possible because all the interaction between Web Inspector front end and the inspected script is going to be serialized into JSON strings. To make that possible there is going to be a small JS piece of Web Inspector(called injected script) sitting in the inspected script and having the same privileges as the inspected script. The injected script will be passed InjectedScriptHost instance that will provide it with interface to the Web Inspector front end. All the methods of InjectedScriptHost are supposed to accept only simple type parameters(strings, numbers, booleans, no objetcs) so that no JS objects leak from the inspected context to the Web Inspector front end. It should be safe.
 

> +    void discardInjectedScripts();
> 
> This function is never called.
>
The interface is intentionally not used yet. I'm going to send V8 implementation in a separate change and after that land the code that uses the new methods(bug 32554).

> Will m_idToInjectedScript grow unbounded?
>
Yes.

> +    m_nextInjectedScriptId++;
> 
> Eventually, this identifier will wrap around, and may collide with previously
> vended identifiers.
It's very unlikely since there is a separate counter for a page and its value is bounded by the number of inspected JSDOMGlobalObjects in that page. The resource identifier counter e.g. will wrap around before that.


> To fix this, I would recommend using the (intptr_t) value
> of injectedScriptObject as your identifier.
There is no guarantee that new injectedScriptObject won't be allocated at the same address.

[Geoffrey Garen]

> Currently for security reasons Web Inspector works with quarantined objects
> instead of the objects from the inspected script. This patch is a part of a
> bigger change(bug 32554) that will allow to get rid of object quarantine.

OK, thanks for the additional context. This seems like a good plan.

> > +    void discardInjectedScripts();
> > 
> > This function is never called.
> >
> The interface is intentionally not used yet.

OK.

> > +    m_nextInjectedScriptId++;
> > 
> > Eventually, this identifier will wrap around, and may collide with previously
> > vended identifiers.
> It's very unlikely since there is a separate counter for a page and its value
> is bounded by the number of inspected JSDOMGlobalObjects in that page. The
> resource identifier counter e.g. will wrap around before that.
> 
> 
> > To fix this, I would recommend using the (intptr_t) value
> > of injectedScriptObject as your identifier.
> There is no guarantee that new injectedScriptObject won't be allocated at the
> same address.

Since the id is bounded by the number of global objects, I guess it's guaranteed not to wrap around, since you would provably run out of memory before it wrapped around.

Still, it seems strange to alias a long to a long, instead of using the original long.

As long as the old object is alive, a new object will not be allocated in its place. But maybe you're saying that an identifier must remain good even after the object it identifies has ben destroyed?

I have two additional comments about the general design here, which I'd like you to consider resolving in future patches:

1. I don't think "InjectedScript", which is a fairly general phrase, is a good name to give to this technology. I'd prefer a name that conveyed the fact that this technology is only safe to use in limited circumstances. How about "InspectorHost" (inside the inspected page) and "InspectorClient" (the Web Inspector UI).

2. I don't think interfaces like this are safe:

+    ScriptObject injectedScriptFor(ScriptState*);
+    ScriptObject injectedScriptForId(long);

Because you're giving out a generic object, the client of the interface can do anything with the object, including passing it unsafe data. It would be better if the C++ object wrapping the "injected script" controlled all interaction with it, to ensure that no non-primitive data leaked across the boundary.

[Geoffrey Garen]

Comment on attachment 46277 [details]
patch

r=me

[Yury Semikhatsky]

(In reply to comment #6)
> As long as the old object is alive, a new object will not be allocated in its
> place. But maybe you're saying that an identifier must remain good even after
> the object it identifies has ben destroyed?
>
I'd like to avoid reusing the identifiers so that we can detect the cases when already destroyed object is requested(should this happen). Also assigning surrogate identifier to the object will serve both V8 moving GC and JSC non-moving GC.

> I have two additional comments about the general design here, which I'd like
> you to consider resolving in future patches:
> 
> 1. I don't think "InjectedScript", which is a fairly general phrase, is a good
> name to give to this technology. I'd prefer a name that conveyed the fact that
> this technology is only safe to use in limited circumstances. How about
> "InspectorHost" (inside the inspected page) and "InspectorClient" (the Web
> Inspector UI).
> 
I agree with you that the naming is not the best. Currently the inspector front-end delegate is called InjectedScriptHost. We should come up with something more specific.

> 2. I don't think interfaces like this are safe:
> 
> +    ScriptObject injectedScriptFor(ScriptState*);
> +    ScriptObject injectedScriptForId(long);
> 
> Because you're giving out a generic object, the client of the interface can do
> anything with the object, including passing it unsafe data. It would be better
> if the C++ object wrapping the "injected script" controlled all interaction
> with it, to ensure that no non-primitive data leaked across the boundary.
Eventually we will create a typed interface for that object. Currently it is used only as ScriptFunctionCall argument but we should restrict the set of functions that can be called on that object, it would make the injected script interface clearer. We've already done similar thing in InspectorFrontend.cpp

[Yury Semikhatsky]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/JSDOMGlobalObject.cpp
	M	WebCore/bindings/js/JSDOMGlobalObject.h
	M	WebCore/bindings/js/JSInjectedScriptHostCustom.cpp
	M	WebCore/inspector/InjectedScriptHost.cpp
	M	WebCore/inspector/InjectedScriptHost.h
Committed r53119