Attaching a tracing file and also its screenshot. Note the number of times layout gets updated when the button is clicked.

Deleted: Screenshot from 2016-09-13 20:27:33.png 104 KB

	Screenshot from 2016-09-13 20:27:33.png 104 KB View Download

Deleted: trace_repeated-layout.json.gz 1.9 MB

trace_repeated-layout.json.gz 1.9 MB Download

It is the following code that forces a layout recalc everytime |updateStyleAndLayoutIgnorePendingStylesheets()| is called:

} else if (m_hasNodesWithPlaceholderStyle && needsLayoutTreeUpdate()) {
    // If new nodes have been added or style recalc has been done with style sheets still
    // pending, some nodes may not have had their real style calculated yet. Normally this
    // gets cleaned when style sheets arrive but here we need up-to-date style immediately.
    setNeedsStyleRecalc(SubtreeStyleChange, StyleChangeReasonForTracing::create(StyleChangeReason::CleanupPlaceholderStyles));
}

I'm wondering if we really need it. If nothing has changed (no DOM change, no new/pending stylesheet loaded) since the last layout update, then even if we force a style recalc here, we will not be able to clear any placeholder style or get anything new. On the other hand, if a change really happened, then we should have already marked the layout tree dirty, and do not need to force a layout here.

I tried removing this else-branch locally. It didn't break any layout test.

Uploaded the patch, which passed CQ dry run: https://codereview.chromium.org/2333263002

+esprehn

So I think the bug here is that needsLayoutTreeUpdate is true every time, that shouldn't be the case. After a call to updateStyleAndLayoutTree(), needsLayoutTreeUpdate() should return false.

Which bit in there is not getting cleared?
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.cpp?q=needsLayoutTreeUpdate&sq=package:chromium&l=1447

This might be a style sheet processing bug where the @import is making us think the sheets are loading repeatedly?

Hmm, the code doesn't look like what's pasted above, the check is actually just:

} else if (m_hasNodesWithPlaceholderStyle) {
    setNeedsStyleRecalc(SubtreeStyleChange, StyleChangeReasonForTracing::create(StyleChangeReason::CleanupPlaceholderStyles));
}

I'm not sure where xiaochengh@ saw the needsLayoutTreeUpdate check.

Anyway here's a reduction:

requestAnimationFrame(function() {
    for (var i = 0; i < 200; ++i) {
        var div = document.createElement('div');
        div.textContent = 'foo bar.';
        document.body.appendChild(div);
    }

    // @import is required.
    var style = document.createElement('style');
    style.textContent = '@import url(-)';
    document.body.appendChild(style);

    // Do a layout with pending sheets.
    document.body.offsetTop;

    // HTML Import is required.
    var importElement = document.createElement('link');
    importElement.setAttribute('rel', 'import');
    document.body.appendChild(importElement);

    var t = performance.now();
    for (var i = 0; i < 5000; ++i) {
    		// Blink performs layout every time!
        document.body.offsetTop;
    }
    console.log('Should a be small number: ' + (performance.now() - t));
});

Looks like some interaction between @import and HTML imports, I think the bug is actually here:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.h?type=cs&sq=package:chromium&rcl=1473794312&l=401

isRenderingReady() is haveImportsLoaded() && haveRenderBlockingStylesheetsLoaded();

haveRenderBlockingStylesheetsLoaded is true if ignorePendingStylesheets is true:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/StyleEngine.h?type=cs&sq=package:chromium&rcl=1473794312&l=112

but haveImportsLoaded() isn't, so what happens is that we end up here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp?q=StyleResolver.cpp&sq=package:chromium&l=721

and isRenderingReady() is false, and any display: none thing will not have a layout object (the <style> or the <link> in this test case). So we produce a *new* placeholder style which sets m_hasNodesWithPlaceholderStyle so the next time through we do another full document style recalc and the process repeats.

isRenderingReady() needs to return true when imports are still pending if ignorePendingStylesheets is set.

Here is Elliott's repro in a clicky: https://jsfiddle.net/8uvu73sv/ My Macbook Air has timing numbers of ~1000, Firefox has ~2.

This is present on stable (Chrome 52), so it's a not a recent regression. It also does not repro in Firefox..

Test team - please use the above test for bisecting, thanks!

This doesn't need a bisect, this is just a bug from adding support for imports. Someone needs to just go fix it.

Whoops, I pasted the wrong code in #2... The code base should look like #6, which doesn't have the |needsLayoutTreeUpdate()| check.

I'll upload a patch with the approach in #6, and also two test cases: one following the discussion in https://codereview.chromium.org/2333263002/, and the other is a regression test for this issue.

Patch updated and waiting for review: https://codereview.chromium.org/2333263002

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

commit cd626f11b0ddb8fc655ac2fd808ca3930e3e29cf
Author: xiaochengh <xiaochengh@chromium.org>
Date: Fri Sep 16 01:16:02 2016

Document::haveImportsLoaded() should return true when ignoring pending sheets

BUG= 646323 
TESTS=webkit_unit_tests DocumentLoadingRenderingTest.ShouldNotPerformRepeatedLayoutWithPendingImport DocumentLoadingRenderingTest.ShouldClearPlaceholderStyleWhenIgnoringPendingStylesheet

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

[modify] https://crrev.com/cd626f11b0ddb8fc655ac2fd808ca3930e3e29cf/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/cd626f11b0ddb8fc655ac2fd808ca3930e3e29cf/third_party/WebKit/Source/web/tests/DocumentLoadingRenderingTest.cpp

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

commit 593d84c9016da731e62991eec28d998104b121da
Author: xiaochengh <xiaochengh@chromium.org>
Date: Wed Sep 21 10:44:07 2016

Revert "Document::haveImportsLoaded() should return true when ignoring pending sheets"

This is a manual revert of commit cd626f11b0ddb8fc655ac2fd808ca3930e3e29cf
for causing  crbug.com/648547 .

It also marks editing/selection/modify_move/move_backward_line_import_crash.html
as crashing, because this layout test is a repro case of  crbug.com/646323 .

Original issue's description:
> Document::haveImportsLoaded() should return true when ignoring pending sheets
>
> BUG= 646323 
> TESTS=webkit_unit_tests DocumentLoadingRenderingTest.ShouldNotPerformRepeatedLayoutWithPendingImport DocumentLoadingRenderingTest.ShouldClearPlaceholderStyleWhenIgnoringPendingStylesheet
>
> Committed: https://crrev.com/cd626f11b0ddb8fc655ac2fd808ca3930e3e29cf
> Cr-Commit-Position: refs/heads/master@{#419067}

BUG= 648547 , 646323 
TBR=esprehn@chromium.org,rune@chromium.org

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

[modify] https://crrev.com/593d84c9016da731e62991eec28d998104b121da/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/593d84c9016da731e62991eec28d998104b121da/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/593d84c9016da731e62991eec28d998104b121da/third_party/WebKit/Source/web/tests/DocumentLoadingRenderingTest.cpp

Reopening this issue since r419067 caused a regression in  issue 648547 .

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

commit 62e3d20e14e91f3999e3ec92fc9f706f9b29d21b
Author: fs <fs@opera.com>
Date: Wed Sep 21 13:48:04 2016

Widen Mac expectations for move_backward_line_import_crash.html

editing/selection/modify_move/move_backward_line_import_crash.html
appears to also Crash on Mac Debug.

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.11%20%28dbg%29/builds/4772

TBR=xiaochengh@chromium.org
BUG= 648547 , 646323 
NOTRY=true

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

[modify] https://crrev.com/62e3d20e14e91f3999e3ec92fc9f706f9b29d21b/third_party/WebKit/LayoutTests/TestExpectations

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

commit 79c1db973587ff78fe06785dd12101cc896d1645
Author: xiaochengh <xiaochengh@chromium.org>
Date: Fri Oct 14 07:20:18 2016

Unmark editing/selection/modify_move/move_backward_line_import_crash.html as failing

This patch removes the failing expectation of the layout test since
it no longer fails.

BUG= 646323 

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

[modify] https://crrev.com/79c1db973587ff78fe06785dd12101cc896d1645/third_party/WebKit/LayoutTests/TestExpectations

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

commit d7c76ec65a164e8b0d1bcede4f6763026ce29042
Author: yosin <yosin@chromium.org>
Date: Wed Oct 26 04:04:48 2016

Introduce EditCommand::setEndingSelection() taking SelectionInDOMTree

This patch introduces |SelectionInDOMTree| version of |setEndingSelection()| in
|EditCommand| class as a preparation of getting rid of overloads and update
layout calls for improving code health.

Following patch will introduce SelectionInUndoStep which is intended for
restore selection after DOM tree mutation at undo/redo time.

BUG=648949,  646323 
TEST=n/a; no behavior changes

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

[modify] https://crrev.com/d7c76ec65a164e8b0d1bcede4f6763026ce29042/third_party/WebKit/Source/core/editing/commands/EditCommand.cpp
[modify] https://crrev.com/d7c76ec65a164e8b0d1bcede4f6763026ce29042/third_party/WebKit/Source/core/editing/commands/EditCommand.h

Unassigning myself since this no longer blocks issue 590369.

The previous attempt caused regression. I don't have enough knowledge to fix this bug.

The issue seems fixed.

Tested with https://jsfiddle.net/xiaochengh/8uvu73sv/1/. The original repro in #7 no longer works as it throws 404. The updated fiddle uses some (randomly found) real stylesheet.

No repeated layout was observed. The 5000 calls of document.body.offsetTop were done in ~3ms.