New issue
Advanced search Search tips

Issue 754708 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug-Regression



Sign in to add a comment

captureScreenshot doesn't work with deviceScaleFactor set to 0

Reported by belym.a....@gmail.com, Aug 11 2017

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.90 Safari/537.36

Steps to reproduce the problem:
1. Start Chrome with debugging protocol: chrome --headless --remote-debugging-port=9222 

2. Connect to Chrome with chrome-remote-interface NPM package

3. Call Emulation.setDeviceMetricsOverride({ width: $SOME_WIDTH, height: $SOME_HEIGHT, mobile: $TRUE_OR_FALSE, deviceScaleFactor: 0}). Docs says zero is a valid value for deviceScaleFactor (means that it disables override) https://chromedevtools.github.io/devtools-protocol/tot/Emulation/#method-setDeviceMetricsOverride

4. Call Page.enable() and then Page.captureScreenshot({fromSurface: true}). The captureScreenshot execution never ends and it doesn't give any result or exception. Meanwhile the page client area turns grey.

What is the expected behavior?
Page.captureScreenshot should finish execution and return an image. 

What went wrong?
Page.captureScreenshot hangs when there was a prior call to Emulation.setDeviceMetricsOverride with deviceScaleFactor: 0. It works when explicitly setting deviceScaleFactor (e.g. deviceScaleFactor: 1).

Did this work before? Yes 60.0.3112.90

Chrome version: 62.0.3182.0  Channel: stable
OS Version: 10.0
Flash Version: Shockwave Flash 26.0 r0
 
Screenshots of a window before and after a call to captureScreenshot.

Script for Node.js to reproduce (Node >=8.x.x):

const cri = require('chrome-remote-interface');

(async () => {
    const client = await cri();
    
    await client.Page.enable();
    await client.Page.navigate({ url: 'http://google.com' });
    
    await client.Emulation.setDeviceMetricsOverride({ 
        width: 800, 
        height: 600, 
        mobile: false, 
        deviceScaleFactor: 0 
    });
    
    // Without a timeout an exception is raised: Error: Unable to capture screenshot
    await new Promise(resolve => setTimeout(resolve, 2000));
    
    // Hangs here \/ \/ \/
    const screenshotData = await client.Page.captureScreenshot({ fromSurface: true });
})();
scr1.png
167 KB View Download
scr2.png
10.6 KB View Download
Owner: dgozman@chromium.org
Status: Assigned (was: Unconfirmed)
Cc: dgozman@chromium.org
Owner: pfeldman@chromium.org
Pavel, mind taking a look?
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cab5c4e97f43a69de32b2d1749d38997a0a6a1a7

commit cab5c4e97f43a69de32b2d1749d38997a0a6a1a7
Author: Pavel Feldman <pfeldman@chromium.org>
Date: Sat Aug 12 01:34:17 2017

Make screnshots work for emulation with default devicescalefactor.

Bug:  754708 
Change-Id: I35a5efedcbf330d793c85dc695523ecd6db588d0
Reviewed-on: https://chromium-review.googlesource.com/611796
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Pavel Feldman <pfeldman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493928}
[modify] https://crrev.com/cab5c4e97f43a69de32b2d1749d38997a0a6a1a7/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
[modify] https://crrev.com/cab5c4e97f43a69de32b2d1749d38997a0a6a1a7/content/browser/devtools/protocol/page_handler.cc

Cc: pfeldman@chromium.org
 Issue 755653  has been merged into this issue.
Labels: -Pri-2 ReleaseBlock-Stable M-61 Merge-Request-61 Pri-1
Requesting merge to M61. Manually verified on Canary.
Project Member

Comment 7 by sheriffbot@chromium.org, Aug 16 2017

Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
This bug requires manual review: M61 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 8 by gov...@chromium.org, Aug 16 2017

Before we approve merge to M61, please answer followings:
* Is this M61 regression? Is it critical?
* Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M61?
* Any other important details to justify the merge.

Please note We're only few weeks away from M61 Stable promotion, so merge bar is very high. Thank you.

Note this is breaking capturing screenshots in DevTools Device Mode (when not overriding dpr). It's a regression, live in Canaray for a while, covered with tests.
Labels: -Merge-Review-61 Merge-Approved-61
Approving merge to M61 branch 3163 based on comment #6 and #9. 
Please merge you change to M61 branch 3163 by 4:00 PM PT tomorrow, Thursday (08/17) so we can take it in for next week Beta release. Thank you.
Labels: -Merge-Approved-61
Status: Fixed (was: Assigned)
Turned out this has merge conflict with https://chromium.googlesource.com/chromium/src/+/452bcd1e19ce87fa9f7bb754d3f202f3780932a6. I think we'll skip the merge to M61.
Labels: -ReleaseBlock-Stable
What version did this land in?  It looks like it wasn't merged into M61 but its still marked as such.  I'm seeing similar issues today when trying to use deviceScaleFactor of higher than 1.

Sign in to add a comment