Closed Bug 1370467 Opened 4 years ago Closed 4 years ago
(Nightly) Screenshot Node in element inspector doesn't work
59 bytes, text/x-review-board-request
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
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
:ochameau is on PTO, let's make sure :jdescottes is aware as well.
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 : > 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.  http://searchfox.org/mozilla-central/rev/d441cb24482c2e5448accaf07379445059937080/devtools/shared/gcli/source/lib/gcli/types/node.js#91
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
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 firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/1766ee2089e8 avoid CPOW in screenshot gcli command;r=pbro
You need to log in before you can comment on or make changes to this bug.