Closed Bug 1370467 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: Inspector, defect, P2)

55 Branch
defect

Tracking

(firefox-esr52 unaffected)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox-esr52 --- unaffected

People

(Reporter: pudgepacket, Assigned: jdescottes)

References

Details

(Keywords: nightly-community, regression)

Attachments

(1 file)

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.
Summary: Screenshot Node in element inspector doesn't work → (Nightly) Screenshot Node in element inspector doesn't work
Confirmed with the same build on Linux
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools
Ever confirmed: true
Component: Developer Tools → Developer Tools: Inspector
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
Blocks: 1355994
Flags: needinfo?(poirot.alex)
:ochameau is on PTO, let's make sure :jdescottes is aware as well.
Flags: needinfo?(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
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Flags: needinfo?(poirot.alex)
Flags: needinfo?(jdescottes)
Priority: -- → P2
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.)
(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
Attachment #8875057 - Flags: review?(jryans) → review?(pbrosset)
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!
Attachment #8875057 - Flags: review?(pbrosset) → review+
thanks for the review! Try is green, landing.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1766ee2089e8
avoid CPOW in screenshot gcli command;r=pbro
https://hg.mozilla.org/mozilla-central/rev/1766ee2089e8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.