Invalidate throttling-related dirty bits when changing cross-origin status.

Invalidate throttling-related dirty bits when changing cross-origin status.

Changing whether a frame is cross-origin affects render throttling. We need
to invalidate when it changes, in particular when updating document.domain
from script.

BUG=691569
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2692993003
Cr-Commit-Position: refs/heads/master@{#451041}
Committed: https://chromium.googlesource.com/chromium/src/+/c652c83fd35923669bfe64c7ec436977bbfed79d

[chrishtr, 2017-02-14 00:31:31]

Description was changed from

==========
none


none


none


none


none

BUG=
==========

to

==========
Invalidate throttling-related dirty bits when changing cross-origin status.

Changing whether a frame is cross-origin affects render throttling. We need
to invalidate when it changes, in particular when updating document.domain
from script.

BUG=691569
==========

[chrishtr, 2017-02-14 00:31:43]

The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 00:33:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 00:37:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 00:37:11]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[chrishtr, 2017-02-15 00:20:12]

chrishtr@chromium.org changed reviewers:
+ pdr@chromium.org

[chrishtr, 2017-02-15 00:20:17]

The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 00:20:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 03:28:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 03:28:29]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2017-02-16 02:05:37]

chrishtr@chromium.org changed reviewers:
+ skyostil@chromium.org

[chrishtr, 2017-02-16 02:05:38]

seems ok?

[pdr., 2017-02-16 04:04:33]

LGTM

https://codereview.chromium.org/2692993003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2692993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4656: if (view())
WDYT of changing this to only make the call when needed?
bool wasCrossDomain = m_frame->isCrossOriginSubframe();
getSecurityOrigin()->setDomainFromDOM(newDomain);
if (view() && (wasCrossDomain != m_frame->isCrossOriginSubframe()))
  view()->crossOriginStatusChanged();

I think the root issue is due to SecurityOrigin's m_domainWasSetInDOM which can
change the cross origin bit (see comment in SecurityOrigin::canAccess).

[pdr., 2017-02-16 04:12:16]

Can you remove the temporary conditional added in PaintLayerClipper as well?

[chrishtr, 2017-02-16 04:18:48]

Description was changed from

==========
Invalidate throttling-related dirty bits when changing cross-origin status.

Changing whether a frame is cross-origin affects render throttling. We need
to invalidate when it changes, in particular when updating document.domain
from script.

BUG=691569
==========

to

==========
Invalidate throttling-related dirty bits when changing cross-origin status.

Changing whether a frame is cross-origin affects render throttling. We need
to invalidate when it changes, in particular when updating document.domain
from script.

BUG=691569
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-02-16 04:25:17]

On 2017/02/16 at 04:12:16, pdr wrote:
> Can you remove the temporary conditional added in PaintLayerClipper as well?

I'd prefer to do that in a followup CL. There may be additional sources of
under-invalidation and I want this fix to stick.

[chrishtr, 2017-02-16 04:25:25]

https://codereview.chromium.org/2692993003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2692993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4656: if (view())
On 2017/02/16 at 04:04:33, pdr. wrote:
> WDYT of changing this to only make the call when needed?
> bool wasCrossDomain = m_frame->isCrossOriginSubframe();
> getSecurityOrigin()->setDomainFromDOM(newDomain);
> if (view() && (wasCrossDomain != m_frame->isCrossOriginSubframe()))
>   view()->crossOriginStatusChanged();

Ok.

> 
> I think the root issue is due to SecurityOrigin's m_domainWasSetInDOM which
can change the cross origin bit (see comment in SecurityOrigin::canAccess).

Correct, this is the root issue.

[chrishtr, 2017-02-16 04:25:33]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-02-16 04:25:33]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2692993003/#ps60001
(title: "none")

[commit-bot: I haz the power, 2017-02-16 04:26:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 06:15:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 06:15:18]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Sami, 2017-02-16 14:48:10]

lgtm, thanks for the fix!

[chrishtr, 2017-02-16 16:29:57]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-02-16 16:30:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 19:16:24]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487262597124130,
"parent_rev": "630890d946fe48d816d3e409e6607dedac1552ba", "commit_rev":
"c652c83fd35923669bfe64c7ec436977bbfed79d"}

[commit-bot: I haz the power, 2017-02-16 19:17:12]

Description was changed from

==========
Invalidate throttling-related dirty bits when changing cross-origin status.

Changing whether a frame is cross-origin affects render throttling. We need
to invalidate when it changes, in particular when updating document.domain
from script.

BUG=691569
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Invalidate throttling-related dirty bits when changing cross-origin status.

Changing whether a frame is cross-origin affects render throttling. We need
to invalidate when it changes, in particular when updating document.domain
from script.

BUG=691569
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2692993003
Cr-Commit-Position: refs/heads/master@{#451041}
Committed:
https://chromium.googlesource.com/chromium/src/+/c652c83fd35923669bfe64c7ec43...
==========

[commit-bot: I haz the power, 2017-02-16 19:17:14]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c652c83fd35923669bfe64c7ec43...