Bug 106811 – Web Inspector: Add iframe option to inspectedWindow.eval() extension API

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

[johnjbarton]

The extension API function inspectedWindow.eval() allows tooling extensions to gather data from the web page, critical to many extensions. Currently extensions cannot access data from iframes however, limiting extensions to simple web pages.

By adding another option to the existing API, iframes can be supported. Here is example code from a test case. First we get the full URL from inspectedWindow.getResources:
var iframeSrc = document.querySelector('iframe').getAttribute('src');
    var url;
    webInspector.inspectedWindow.getResources(function(resources){
        resources.foreach(function(resource){
            if (resource.url.indexOf(iframeSrc) !== -1) {
                url = resource.url;
            }
        });
    });

Then we extract the origin:

    var segments = url.split('/'); // http://a.b.c:xxx/path
    var origin = segments[0] + '//' + segments[1];

And use these in the option to eval:
    var options = {
        frame: {
            url: url,
            securityOrigin: origin
        }
    }
    webInspector.inspectedWindow.eval("window.location.pathname", options, callbackAndNextTest(extension_onEval, nextTest));

contentScript contexts could be accessed using their securityOrigin.

Patch ready

[johnjbarton]

Created attachment 182649 [details]
Patch

[Build Bot]

Comment on attachment 182649 [details]
Patch

Attachment 182649 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15870739

New failing tests:
inspector/extensions/extensions-eval.html

[WebKit Review Bot]

Comment on attachment 182649 [details]
Patch

Attachment 182649 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15874693

New failing tests:
inspector/extensions/extensions-eval.html

[WebKit Review Bot]

Comment on attachment 182649 [details]
Patch

Attachment 182649 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15872718

New failing tests:
inspector/extensions/extensions-eval.html

[Build Bot]

Comment on attachment 182649 [details]
Patch

Attachment 182649 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15913117

New failing tests:
inspector/extensions/extensions-eval.html

[Andrey Kosyakov]

Comment on attachment 182649 [details]
Patch

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

> Source/WebCore/inspector/front-end/ExtensionServer.js:801
> +            } else if (options.frame && typeof options.frame === "object" && options.frame.url && options.frame.securityOrigin) {

can we support options.frame and options.useContentScriptContext at the same time? I can imagine this may be useful.

> Source/WebCore/inspector/front-end/ExtensionServer.js:810
> +                            context = contextLists[i].contextBySecurityOrigin(options.frame.securityOrigin);    

So this will allow an extension to evaluate in content script of other extensions. This may be a bit more than we would like to allow now. How about just using frame.url and handling useContentScriptContext.

> Source/WebCore/inspector/front-end/ExtensionServer.js:816
> +                    callback(errorMessage);

this should not be necessary, if you return an error status, the caller will invoke callback.

> Source/WebCore/inspector/front-end/ExtensionServer.js:817
> +                    return this._status.E_FAILED(errorMessage);

I think E_NOTFOUND is more appropriate here, E_FAILED is too generic.

> LayoutTests/inspector/extensions/extensions-eval-expected.txt:19
> +log: Extension server error: Operation failed: No context with URL: "file:///work/chromium/src/third_party/WebKit/LayoutTests/inspector/extensions/resources/extensions-frame-eval.html" and origin "bogus"

Well.. That's not going to ever pass on the bots :)

> LayoutTests/inspector/extensions/extensions-eval.html:49
> +function extension_testZEvalInIFrame(nextTest) {

Z?

> LayoutTests/inspector/extensions/extensions-eval.html:50
> +    var iframeSrc = 'resources/extensions-frame-eval.html';

I think a good test would be to try something cross-origin -- i.e. move it under http tests and use http://127.0.0.1:8000/ as on origin.

[johnjbarton]

(In reply to comment #6)
> (From update of attachment 182649 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182649&action=review
> 
> > Source/WebCore/inspector/front-end/ExtensionServer.js:801
> > +            } else if (options.frame && typeof options.frame === "object" && options.frame.url && options.frame.securityOrigin) {
> 
> can we support options.frame and options.useContentScriptContext at the same time? I can imagine this may be useful.

Yes, good idea.

> 
> > Source/WebCore/inspector/front-end/ExtensionServer.js:810
> > +                            context = contextLists[i].contextBySecurityOrigin(options.frame.securityOrigin);    
> 
> So this will allow an extension to evaluate in content script of other extensions. This may be a bit more than we would like to allow now. How about just using frame.url and handling useContentScriptContext.

It allows a devtools extension to evaluate in the context of another browser extension. Two browser extensions are 'peers': they have equivalent possible permissions and powers. Allowing browser extensions to operate on other browser extensions could lead to confusing interference. 

Devtools extensions are not peers of browser extensions: they are explicitly -- by the specific user opening devtools -- granted extra power -- eg the ability to eval() directly into the context of web page. Shouldn't devtools extensions should be able to eval() into browser extensions just like devtools can?

> 
> > Source/WebCore/inspector/front-end/ExtensionServer.js:816
> > +                    callback(errorMessage);
> 
> this should not be necessary, if you return an error status, the caller will invoke callback.
> 
> > Source/WebCore/inspector/front-end/ExtensionServer.js:817
> > +                    return this._status.E_FAILED(errorMessage);
> 
> I think E_NOTFOUND is more appropriate here, E_FAILED is too generic.

Actually the opposite was my experience: I started with E_NOTFOUND but I could not figure out what to do with an error message like 'Not found "http://localhost:9696"'. I realize that E_NOTFOUND makes better documentation in the source code but the resulting error message is not as good for the extension author.

> 
> > LayoutTests/inspector/extensions/extensions-eval-expected.txt:19
> > +log: Extension server error: Operation failed: No context with URL: "file:///work/chromium/src/third_party/WebKit/LayoutTests/inspector/extensions/resources/extensions-frame-eval.html" and origin "bogus"
> 
> Well.. That's not going to ever pass on the bots :)

It would if everyone switched to my file system layout ;-)  Too much cut and paste I can see.

> 
> > LayoutTests/inspector/extensions/extensions-eval.html:49
> > +function extension_testZEvalInIFrame(nextTest) {
> 
> Z?

For some reason the test issues tests.sort() and I wanted this test to run last.

> 
> > LayoutTests/inspector/extensions/extensions-eval.html:50
> > +    var iframeSrc = 'resources/extensions-frame-eval.html';
> 
> I think a good test would be to try something cross-origin -- i.e. move it under http tests and use http://127.0.0.1:8000/ as on origin.

Sure.

Let me know about the other issues here and I will resubmit the patch.

Thanks!

[johnjbarton]

(In reply to comment #6)
> 
> > Source/WebCore/inspector/front-end/ExtensionServer.js:816
> > +                    callback(errorMessage);
> 
> this should not be necessary, if you return an error status, the caller will invoke callback.
> 
That turned out to be a good hint...
> 
> > LayoutTests/inspector/extensions/extensions-eval.html:49
> > +function extension_testZEvalInIFrame(nextTest) {
> 
> Z?

These two issues were related. ExtensionServer.js was not correctly propagating errors it detected before sending the request to the debug server because the caller for evaluate() in this case, _onEvaluateOnInspectedPage(), was not invoking the callback.  That allowed the test driver to launch nextTest() on both the main-line and async callback line, which resulted in duplicate test entries that I was trying to hackaround with Z (so the test case that issued dupes was last and there were no more tests to dupe).

[johnjbarton]

Created attachment 183058 [details]
Patch

[johnjbarton]

Ok this patch has all of the things you asked for *except* it still includes options.frame.securityOrigin which I argued for in comment 7.

This patch will need to be rebased after bug 106926 lands.

[Andrey Kosyakov]

> 
> Devtools extensions are not peers of browser extensions: they are explicitly -- by the specific user opening devtools -- granted extra power -- eg the ability to eval() directly into the context of web page. Shouldn't devtools extensions should be able to eval() into browser extensions just like devtools can?
> 

Well, we did not enable this originally, but this was perhaps just overcautiousness. I discussed this again with Pavel, and the consensus was -- let's do it!

> > 
> > I think E_NOTFOUND is more appropriate here, E_FAILED is too generic.
> 
> Actually the opposite was my experience: I started with E_NOTFOUND but I could not figure out what to do with an error message like 'Not found "http://localhost:9696"'. I realize that E_NOTFOUND makes better documentation in the source code but the resulting error message is not as good for the extension author.

My primary design goal for error reporting was making the errors easy to process programmatically and (at least in theory) possible to localize. So aside from a the last resort, catch-all E_FAILED that allows arbitrary text, other errors don't expect user-friendly text as parameters. So I'd suggest the following options:
- return E_NOTFOUND for frame if you did not find frame, E_NOTFOUND for context if you did not find context within the frame; optionally, add a console.warn() if you feel extension authors may be confused;
- introduce specific error code for the situation, along with verbose error message;

[Andrey Kosyakov]

Comment on attachment 183058 [details]
Patch

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

> Source/WebCore/inspector/front-end/ExtensionServer.js:798
> +            if (options["useContentScriptContext"] === true) {

we use to check for just options["useContentScriptContext"]

> Source/WebCore/inspector/front-end/ExtensionServer.js:801
> +                frameSecurityOrigin = options.frame.securityOrigin;

Let's put securityOrigin on the top level of options. It's orthogonal to frame, you can also use it for top frame, i.e. without frame selector.
Also, flat options object will be easier to document. So I suggest three options on the top level:

- useContentScriptContext
- scriptExecutionContext (maps to your frame.securityOrigin; either overrides useContentScriptContext or mutually exclusive with it)
- frameURL (maps to frame.url)

[johnjbarton]

Created attachment 185025 [details]
Patch

[johnjbarton]

Rebased and addressed the review in comment 12 so I think this should be ready to go

[Andrey Kosyakov]

Comment on attachment 185025 [details]
Patch

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

> Source/WebCore/ChangeLog:91
>          (WebCore::RenderBox::exclusionShapeOutsideInfo): Ditto.
>  
> +        * inspector/front-end/ExtensionServer.js:

Looks like a merge error.

> Source/WebCore/inspector/front-end/ExtensionServer.js:348
> +        var status = this.evaluate(message.expression, true, true, message.evaluateOptions, port._extensionOrigin, callback.bind(this));
> +        if (status) {
> +            callback.call(this, "Extension server error: " + String.vsprintf(status.description, status.details));
> +        }

Why is this necessary? I don't think this will pass type validation by closure compiler.

> Source/WebCore/inspector/front-end/ExtensionServer.js:794
> +            } else if (options.scriptExecutionContext) {

Please drop redundant braces.

> Source/WebCore/inspector/front-end/ExtensionServer.js:797
> +            var url;

Redundant?

> Source/WebCore/inspector/front-end/ExtensionServer.js:803
> +                    if (contextLists[i].url === url) {

I would expect options.frameURL to be used to resolve frame (perhaps with the help of ResourceTreeModel), then use contextByFrameAndSecurityOrigin()

> Source/WebCore/inspector/front-end/ExtensionServer.js:804
> +                        if (!contextSecurityOrigin || contextSecurityOrigin && url.indexOf(contextSecurityOrigin) === 0) { 

if you choose to iterate over context list, the frame URL should be compared against the URL of the frame the context belongs to, not against the security origin.

> Source/WebCore/inspector/front-end/ExtensionServer.js:816
> +            } else if (contextSecurityOrigin) {

This starts to look a bit complicated. I would suggest something like:
var frame = options.frameURL ? resolveURLToFrame(options.frameURL) : WebInspector.resourceTreeModel.mainFrame;
if (!frame)
    return this._status.E_NOTFOUND(options.frameURL || "<top>")
if (contextSecurityOrigin) {
    context = WebInspector.runtimeModel.contextByFrameAndSecurityOrigin(frame, contextSecurityOrigin);
    if (!context)
         return this._status.E_NOTFOUND(contextSecurityOrigin)
}

> LayoutTests/ChangeLog:108
> +        * http/tests/inspector/extensions-eval-expected.txt: Renamed from LayoutTests/inspector/extensions/extensions-eval-expected.txt.
> +        * http/tests/inspector/extensions-eval.html: Renamed from LayoutTests/inspector/extensions/extensions-eval.html.
> +        * http/tests/inspector/resources/extensions-frame-eval.html: Added.

Merge error?

[johnjbarton]

(In reply to comment #15)
> (From update of attachment 185025 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185025&action=review
> 
> > Source/WebCore/ChangeLog:91
> >          (WebCore::RenderBox::exclusionShapeOutsideInfo): Ditto.
> >  
> > +        * inspector/front-end/ExtensionServer.js:
> 
> Looks like a merge error.

I don't really understand the ChangeLog entry gets created by webkit-patch upload. I'll try to get it to start over.

> 
> > Source/WebCore/inspector/front-end/ExtensionServer.js:348
> > +        var status = this.evaluate(message.expression, true, true, message.evaluateOptions, port._extensionOrigin, callback.bind(this));
> > +        if (status) {
> > +            callback.call(this, "Extension server error: " + String.vsprintf(status.description, status.details));
> > +        }
> 
> Why is this necessary? 

On error, this.evaluate() doe not callback() but returns errors, eg 
   return this._status.E_NOTFOUND(contextSecurityOrigin);
If the caller does not process the return value, these error messages are lost, and the callback never fires.

> I don't think this will pass type validation by closure compiler.

I corrected the type signature of the callback.

> 
> > Source/WebCore/inspector/front-end/ExtensionServer.js:794
> > +            } else if (options.scriptExecutionContext) {
> 
> Please drop redundant braces.

done

> 
> > Source/WebCore/inspector/front-end/ExtensionServer.js:797
> > +            var url;
> 
> Redundant?
yup

The next patch is quite different for the rest of the comments.

[johnjbarton]

Created attachment 185286 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 185286 [details]
Patch

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

> Source/WebCore/inspector/front-end/ExtensionServer.js:348
> +        var status = this.evaluate(message.expression, true, true, message.evaluateOptions, port._extensionOrigin, callback.bind(this));
> +        if (status) {
> +            callback.call(this, "Extension server error: " + String.vsprintf(status.description, status.details));
> +        }

I still think this should not be necessary: the convention is that if the error condition is immediately detect by a handler, the handler returns the error and then ExtensionServer::_onmessage will dispatch the error to the client callback.

> Source/WebCore/inspector/front-end/ExtensionServer.js:795
> +                WebInspector.resourceTreeModel.forAllFrames(function(frame) {

Can we just resturn frames array from ResourceTreeModel. I think it would require fewer callbacks and make code more readable.

> LayoutTests/http/tests/inspector/extensions-eval.html:54
> +function extension_testEvalInIFrame(nextTest) {

Can you instead move it into a separate test file, like we do extensions-eval-content-script? Or, perhaps, just add it to the latter and move it to http?
Also, { needs to be on the next line.

> LayoutTests/http/tests/inspector/extensions-eval.html:55
> +    var options = extension_options();

I'd rather inline both options and loc for better readability, i.e.
var options = {
...
}
eval("window.location.pathname", options, callbacnAndNextTest(...));

> LayoutTests/http/tests/inspector/extensions-eval.html:75
> +    var segments = url.split('/'); // http://a.b.c:xxx/path
> +    var origin = segments[0] + '//' + segments[2];

I think just hard-coding it would make it more readable.

[johnjbarton]

(In reply to comment #18)
> (From update of attachment 185286 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185286&action=review
> 
> > Source/WebCore/inspector/front-end/ExtensionServer.js:348
> > +        var status = this.evaluate(message.expression, true, true, message.evaluateOptions, port._extensionOrigin, callback.bind(this));
> > +        if (status) {
> > +            callback.call(this, "Extension server error: " + String.vsprintf(status.description, status.details));
> > +        }
> 
> I still think this should not be necessary: the convention is that if the error condition is immediately detect by a handler, the handler returns the error and then ExtensionServer::_onmessage will dispatch the error to the client callback.

But without this change the handler never sees the error condition! If evaluate() returns say, E_NOTFOUND, and we ignore the return value (as without this patch) how can _onMessage dispatch an error to the client? It does not even see the error. 

Maybe you prefer a different solution, but I could not get my code to work without some change here.


> 
> > Source/WebCore/inspector/front-end/ExtensionServer.js:795
> > +                WebInspector.resourceTreeModel.forAllFrames(function(frame) {
> 
> Can we just resturn frames array from ResourceTreeModel. I think it would require fewer callbacks and make code more readable.

Because _frames in ResourceTreeModel is not an array, it's a map from frame.id -> frame. We can create an array if you prefer. I was just trying to follow the practice in that file.

...
> > LayoutTests/http/tests/inspector/extensions-eval.html:75
> > +    var segments = url.split('/'); // http://a.b.c:xxx/path
> > +    var origin = segments[0] + '//' + segments[2];
> 
> I think just hard-coding it would make it more readable.

What value should I use then?

[Andrey Kosyakov]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 185286 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=185286&action=review
> > 
> > > Source/WebCore/inspector/front-end/ExtensionServer.js:348
> > > +        var status = this.evaluate(message.expression, true, true, message.evaluateOptions, port._extensionOrigin, callback.bind(this));
> > > +        if (status) {
> > > +            callback.call(this, "Extension server error: " + String.vsprintf(status.description, status.details));
> > > +        }
> > 
> > I still think this should not be necessary: the convention is that if the error condition is immediately detect by a handler, the handler returns the error and then ExtensionServer::_onmessage will dispatch the error to the client callback.
> 
> But without this change the handler never sees the error condition! If evaluate() returns say, E_NOTFOUND, and we ignore the return value (as without this patch) how can _onMessage dispatch an error to the client? It does not even see the error. 
> 

Here's the code that is responsible to dispatching the error to the client:

http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/inspector/front-end/ExtensionServer.js&exact_package=chromium&q=onmessage%20file:extensionserver&l=724

(lines 724, 728 & 729)

You can hit it in existing code if you tweak one of the tests (duh -- we need a test for this as well). If you changle file:/// to something else in this line, you will see that the callback is invoked even upon error:

http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/LayoutTests/inspector/extensions/extensions-eval-content-script.html&exact_package=chromium&q=file:extensions-eval-&type=cs&l=9

That said, the way we report these errors to the client is quite unfortunate. I'm about to fix that, see bug 108640.

> > Can we just resturn frames array from ResourceTreeModel. I think it would require fewer callbacks and make code more readable.
> 
> Because _frames in ResourceTreeModel is not an array, it's a map from frame.id -> frame. We can create an array if you prefer.

I think toy can either return the entire map or perhaps just Object.values()

> I was just trying to follow the practice in that file.

I think forAllResources is implemented this way because the iteration over tree is too complicated to perform on the client side. For iteration over map or array, on the other hand, inversion of control looks like overkill :)


> > > LayoutTests/http/tests/inspector/extensions-eval.html:75
> > > +    var segments = url.split('/'); // http://a.b.c:xxx/path
> > > +    var origin = segments[0] + '//' + segments[2];
> > 
> > I think just hard-coding it would make it more readable.
> 
> What value should I use then?

Just http://127.0.0.1:8000/?

[johnjbarton]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > (From update of attachment 185286 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=185286&action=review
> > > 
> > > > Source/WebCore/inspector/front-end/ExtensionServer.js:348
> > > > +        var status = this.evaluate(message.expression, true, true, message.evaluateOptions, port._extensionOrigin, callback.bind(this));
> > > > +        if (status) {
> > > > +            callback.call(this, "Extension server error: " + String.vsprintf(status.description, status.details));
> > > > +        }
> > > 
> > > I still think this should not be necessary: the convention is that if the error condition is immediately detect by a handler, the handler returns the error and then ExtensionServer::_onmessage will dispatch the error to the client callback.
> > 
> > But without this change the handler never sees the error condition! If evaluate() returns say, E_NOTFOUND, and we ignore the return value (as without this patch) how can _onMessage dispatch an error to the client? It does not even see the error. 
> > 
> 
> Here's the code that is responsible to dispatching the error to the client:
> 
> http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/inspector/front-end/ExtensionServer.js&exact_package=chromium&q=onmessage%20file:extensionserver&l=724
> 
> (lines 724, 728 & 729)
> 
> You can hit it in existing code if you tweak one of the tests (duh -- we need a test for this as well). If you changle file:/// to something else in this line, you will see that the callback is invoked even upon error:
> 
> http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/LayoutTests/inspector/extensions/extensions-eval-content-script.html&exact_package=chromium&q=file:extensions-eval-&type=cs&l=9
> 
> That said, the way we report these errors to the client is quite unfortunate. I'm about to fix that, see bug 108640.

In bug 108846 I added a test for a bad option, that is the dev sends useContentScriptContext option in a case where there is not content script world.  I hope that patch will clarify the error return issue.

[johnjbarton]

Created attachment 187634 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 187634 [details]
Patch

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

> Source/WebCore/inspector/front-end/ExtensionServer.js:332
> +            var result;

This would be gone after rebase :)

> Source/WebCore/inspector/front-end/ExtensionServer.js:789
> +                WebInspector.resourceTreeModel.frames().some(function(frame) {

We avoid non-trivial anonymous functions in other places -- can you please extract it as a named function?

> Source/WebCore/inspector/front-end/ExtensionServer.js:808
> +            else if (options.scriptExecutionContext && frame.url.indexOf(options.scriptExecutionContext) !== 0)

So this will work for arbitrary prefixes of frame.url, including "", "http" etc. Is this the intent?

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:245
> +        var frames = [];
> +        Object.keys(this._frames).forEach(function(frameId){
> +            frames.push(this._frames[frameId]);
> +        }.bind(this));
> +        return frames;

return Object.values(this._frames)

[johnjbarton]

Created attachment 188101 [details]
Fix 4 items from review

[Andrey Kosyakov]

Comment on attachment 188101 [details]
Fix 4 items from review

LGTM

[johnjbarton]

review?

[johnjbarton]

Downstream user feature request:
https://code.google.com/p/chromium/issues/detail?id=177713

[johnjbarton]

Created attachment 192490 [details]
rebase

[Vsevolod Vlasov]

Comment on attachment 192490 [details]
rebase

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

> Source/WebCore/inspector/front-end/ExtensionServer.js:803
> +            if (options["useContentScriptContext"] )

Maybe just options.useContentScriptContext?

[johnjbarton]

Created attachment 193593 [details]
rebase; fix review comment #29

[johnjbarton]

cq?

[WebKit Review Bot]

Comment on attachment 193593 [details]
rebase; fix review comment #29

Clearing flags on attachment: 193593

Committed r146202: <http://trac.webkit.org/changeset/146202>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.