1370467 - (Nightly) Screenshot Node in element inspector doesn't work

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[pudgepacket]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170605030204

Steps to reproduce:

Running Nightly 55.0a1 (2017-06-05) (64-bit).

Open inspector.

Right click any node and choose "Screenshot Node".


Actual results:

Nothing.


Expected results:

An image downloaded that contains the image of the node in question.

Comment 1

[Pascal Chevrel:pascalc (PTO until April 26)]

Confirmed with the same build on Linux

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=70fa0a9a7f6f9a31c8699f6e8111b107c5161ea7&tochange=b894fc314a09ff63d89a373d09a9c33f47efcc07

Regressed by: b894fc314a09	Alexandre Poirot — Bug 1355994 - Stop flagging DevTools sandboxes with an Addon ID. r=kmag

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:ochameau is on PTO, let's make sure :jdescottes is aware as well.

Comment 4

[Julian Descottes [:jdescottes]]

Note that using the gcli command `screenshot test.png --selector body` is also broken.

The inspector relies on the gcli command `screenshot` with the `--selector` option. 
`screenshot` is a command that is supposed to run on the client (runAt: 'client')

However when the `--selector` argument is parsed, it calls [1]:
> context.environment.window.document.querySelectorAll(arg.text);
which creates an unsafe CPOW.

`screenshot` cannot run on the server because it relies on APIs available only on the chrome window and documents (to download a file, create a sound effect etc...)

It used to work because before Bug 1355994, DevTools code was incorrectly allowed to use unsafe CPOWs.

We can either backout Bug 1355994, or fix the CPOW. The node found for `--selector` is only used in order to get its Rect. We would just need to call the server to find the node and call shared/layout/utils:getRect() on it. In any case I think we should try to address this before 55 goes to Beta.

I'll try to have a look tomorrow.

[1] http://searchfox.org/mozilla-central/rev/d441cb24482c2e5448accaf07379445059937080/devtools/shared/gcli/source/lib/gcli/types/node.js#91

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Ah, so GCLI looks up the element when parsing the `selector` arg, since the type is `node`...

There is already a `screenshot_server` command that runs server-side which the client-side `screenshot` dispatches too.  Maybe if you change the param definition for the client command to by a string type, and then specify it as a node type in the server command, perhaps that would resolve it?  (Or just leave it as a string everywhere and manually query for the node yourself on the server.)

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1370467 - avoid CPOW in screenshot gcli command;

Review commit: https://reviewboard.mozilla.org/r/146418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146418/

Comment 7

[Julian Descottes [:jdescottes]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> Ah, so GCLI looks up the element when parsing the `selector` arg, since the
> type is `node`...
> 
> There is already a `screenshot_server` command that runs server-side which
> the client-side `screenshot` dispatches too.  Maybe if you change the param
> definition for the client command to by a string type, and then specify it
> as a node type in the server command, perhaps that would resolve it?  (Or
> just leave it as a string everywhere and manually query for the node
> yourself on the server.)

Thanks, it works! Hadn't realized that we already had a server command here. I think it's fair to use string on the client and node on the server, this way we keep the same behavior as before. Flagged you for review since it's based on your suggestion but let me know if you want me to forward it to someone else.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b07f44bd7eb81f716ab837eb2f6178d015dcab51

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8875057 [details]
Bug 1370467 - avoid CPOW in screenshot gcli command;

https://reviewboard.mozilla.org/r/146418/#review150634

Totally stealing this review request!
Thank you Julian for jumping on this and thanks jryans for helping find a fix.
Let's get this in asap!

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8875057 [details]
Bug 1370467 - avoid CPOW in screenshot gcli command;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146418/diff/1-2/

Comment 10

[Julian Descottes [:jdescottes]]

thanks for the review! Try is green, landing.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1766ee2089e8
avoid CPOW in screenshot gcli command;r=pbro

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1766ee2089e8