New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 689330 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 628043



Sign in to add a comment

Investigate copy constructor of StyleVisualData

Project Member Reported by shend@chromium.org, Feb 7 2017

Issue description

In https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/StyleVisualData.cpp?l=40 , the m_zoom member is not copied, but set to the initial zoom value. Hence, every time StyleVisualData is copied for copy-on-write, the zoom is reset. There's no comment to indicate that this was intended. This needs to be investigated to see if it was intentional.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 9 2017

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

commit 4be275ae7e189294e00a1c65ec9976974b1329de
Author: shend <shend@chromium.org>
Date: Thu Feb 09 06:05:36 2017

Add DCHECK in copy constructor of StyleVisualData for m_zoom.

Currently StyleVisualData::m_zoom is reset to the initial zoom value
every time StyleVisualData is copied using the copy constructor. There
is no indication that this is intentional.

This patch adds a DCHECK to see if there's any case where we trigger
this bug. If not, we will copy m_zoom properly in a separate patch.

Not to be merged in.

BUG= 689330 

Review-Url: https://codereview.chromium.org/2687533004
Cr-Commit-Position: refs/heads/master@{#449215}

[modify] https://crrev.com/4be275ae7e189294e00a1c65ec9976974b1329de/third_party/WebKit/Source/core/style/StyleVisualData.cpp

Project Member

Comment 2 by bugdroid1@chromium.org, Feb 9 2017

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

commit 24835036681c326052b092b9697333ad9f4c2a9b
Author: shend <shend@chromium.org>
Date: Thu Feb 09 23:11:23 2017

Copy StyleVisualData::m_zoom in copy constructor.

Currently StyleVisualData::m_zoom is reset to the initial zoom value
every time StyleVisualData is copied using the copy constructor. There
is no indication that this is intentional.

This patch changes the code to actually copy m_zoom instead of
resetting it. We tried to obtain a repro test case by running trybots
with a DCHECK to see if the bug is reacheable, but the trybots passed.
We also tried to find the commit that introduced this, but the code is
very old and we couldn't trace its origin.

It is most likely a copy and paste error, and even if it is not, this
code is not dependable due to copy elision.

See https://codereview.chromium.org/2687533004 for more details.

BUG= 689330 

Review-Url: https://codereview.chromium.org/2686013002
Cr-Commit-Position: refs/heads/master@{#449451}

[modify] https://crrev.com/24835036681c326052b092b9697333ad9f4c2a9b/third_party/WebKit/Source/core/style/StyleVisualData.cpp

Labels: Update-Monthly

Comment 4 by shend@chromium.org, Feb 13 2017

Status: Fixed (was: Assigned)

Sign in to add a comment