Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2752163003:
Clear pseudoElement from ElementRareData when FirstLetterPseudoElement doesn't have a LayoutObject (Closed)
DescriptionClear pseudoElement from ElementRareData when FirstLetterPseudoElement doesn't have a LayoutObject
This patch edits Element::rebuildPseudoElementLayoutTree() to make sure
it mirrors Element::updatePseudoElement() called in
Element::recaclStyle() by calling Element::updateFirstLetter() in the
case the PseudoElement is firstLetter as is done in
Element::updateFirstLetter() to ensure we don't call
Element::retachLayoutTree() uncoditionally.
Adds crash test and expectation as well.
BUG=700655
Review-Url: https://codereview.chromium.org/2752163003
Cr-Original-Commit-Position: refs/heads/master@{#462373}
Committed: https://chromium.googlesource.com/chromium/src/+/6d202487ff823def4672599f51e9f13880e6b877
Review-Url: https://codereview.chromium.org/2752163003
Cr-Commit-Position: refs/heads/master@{#462705}
Committed: https://chromium.googlesource.com/chromium/src/+/9e923986fcf79015b0ff237c06207648f8606560
Patch Set 1 #
Total comments: 5
Patch Set 2 : Change patch per discussion with esprehn@ #Patch Set 3 : Made changes per esprehn's comments #Patch Set 4 : Removing erroneous test #Patch Set 5 : Change test to be testharness #
Messages
Total messages: 46 (31 generated)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nainar@chromium.org changed reviewers: + esprehn@chromium.org
Clusterfuzz hates me but here as promised. A FIX!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Elliott, Logging our conversation in case I forget by the time I am back. In case this needs to land while I am OOO feel free to have anyone do it. Assuming this is correct that is. Would cc meade@ in this case. But they too are OOO for the same period as me. https://codereview.chromium.org/2752163003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.cpp (left): https://codereview.chromium.org/2752163003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:2065: if (element->needsReattachLayoutTree() || Change as per discussed by esprehn@ and I in IRL: In the case of FirstLetter we need to mirror Element::updateFirstLetter(). We have to delete the old first letter here to set the original text back on the text layoutObject. If we dispose the old first letter after creating the new one we will get incorrect results because we set the first letter back. In the codepath before I landed codereview.chromium.org/2473743003 we would have called updateFirstLetter but now we aren't. Calling reattachLayoutTree unconditionally leads to bad state hence the call not having the correct layoutObject information. This causes us to hit the DCHECK seen in the bug. Logging here for the record as I am OOO and will probably forget.
Description was changed from ========== Clear pseudoElement from ElementRareData when FirstLetterPseudoElement doesn't have a LayoutObject This patch edits Element::reattachPseudoElementLayoutTree() to clear the FirsLetterPseusoElement from the ElementRareData if the associated LayoutObject has been destroyed. This can happen if we change to a float, for example. This mirrors Element::updatePseudoElement() called in Element::recaclStyle(). Adds crash test and expectation as well BUG=700655 ========== to ========== Clear pseudoElement from ElementRareData when FirstLetterPseudoElement doesn't have a LayoutObject This patch edits Element::rebuildPseudoElementLayoutTree() to make sure it mirrors Element::updatePseudoElement() called in Element::recaclStyle() by calling Element::updateFirstLetter() in the case the PseudoElement is firstLetter as is done in Element::updateFirstLetter() to ensure we don't call Element::retachLayoutTree() uncoditionally. Adds crash test and expectation as well. BUG=700655 ==========
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Elliott, Made the changes we discussed ages ago. PTAL? Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm https://codereview.chromium.org/2752163003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css/crash-in-attachFirstLetterTextLayoutObjects.html (right): https://codereview.chromium.org/2752163003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css/crash-in-attachFirstLetterTextLayoutObjects.html:1: <style> Can we add a <!DOCTYPE html> https://codereview.chromium.org/2752163003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2752163003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:2068: else if (element->needsReattachLayoutTree() || I think we often add braces when things start to get cramped like this.
btw do we need to merge this to M57? Crashing on stable would be bad. :P
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Made the changes you asked for. Also I landed my change after the M58 cutoff (March 2 2017) so this is only in Canary - so we are good as long as this change lands before branch point (April 13 2017) :) Totes leveraged the entire 6 weeks didn't I :P https://codereview.chromium.org/2752163003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css/crash-in-attachFirstLetterTextLayoutObjects.html (right): https://codereview.chromium.org/2752163003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css/crash-in-attachFirstLetterTextLayoutObjects.html:1: <style> On 2017/04/06 at 00:08:59, esprehn wrote: > Can we add a <!DOCTYPE html> Done. https://codereview.chromium.org/2752163003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2752163003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:2068: else if (element->needsReattachLayoutTree() || On 2017/04/06 at 00:08:59, esprehn wrote: > I think we often add braces when things start to get cramped like this. Done.
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
The CQ bit was unchecked by nainar@chromium.org
The CQ bit was checked by nainar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2752163003/#ps60001 (title: "Removing erroneous test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nainar@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491458059801970,
"parent_rev": "32139c44445456e6c3d53fc82cc978bf5e0457ad", "commit_rev":
"6d202487ff823def4672599f51e9f13880e6b877"}
Message was sent while issue was closed.
Description was changed from ========== Clear pseudoElement from ElementRareData when FirstLetterPseudoElement doesn't have a LayoutObject This patch edits Element::rebuildPseudoElementLayoutTree() to make sure it mirrors Element::updatePseudoElement() called in Element::recaclStyle() by calling Element::updateFirstLetter() in the case the PseudoElement is firstLetter as is done in Element::updateFirstLetter() to ensure we don't call Element::retachLayoutTree() uncoditionally. Adds crash test and expectation as well. BUG=700655 ========== to ========== Clear pseudoElement from ElementRareData when FirstLetterPseudoElement doesn't have a LayoutObject This patch edits Element::rebuildPseudoElementLayoutTree() to make sure it mirrors Element::updatePseudoElement() called in Element::recaclStyle() by calling Element::updateFirstLetter() in the case the PseudoElement is firstLetter as is done in Element::updateFirstLetter() to ensure we don't call Element::retachLayoutTree() uncoditionally. Adds crash test and expectation as well. BUG=700655 Review-Url: https://codereview.chromium.org/2752163003 Cr-Commit-Position: refs/heads/master@{#462373} Committed: https://chromium.googlesource.com/chromium/src/+/6d202487ff823def4672599f51e9... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6d202487ff823def4672599f51e9...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2802993002/ by foolip@chromium.org. The reason for reverting is: The added test is failing on many platforms due to a mismatch between actual expected (not a crash). BUG=708967.
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Elliott, Changed the test to use testHarness. Should fix the issue. LMK what you think.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by nainar@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491523385286850,
"parent_rev": "1d90c42584511fa8bf91f0eadddfe1634f9523fe", "commit_rev":
"9e923986fcf79015b0ff237c06207648f8606560"}
Message was sent while issue was closed.
Description was changed from ========== Clear pseudoElement from ElementRareData when FirstLetterPseudoElement doesn't have a LayoutObject This patch edits Element::rebuildPseudoElementLayoutTree() to make sure it mirrors Element::updatePseudoElement() called in Element::recaclStyle() by calling Element::updateFirstLetter() in the case the PseudoElement is firstLetter as is done in Element::updateFirstLetter() to ensure we don't call Element::retachLayoutTree() uncoditionally. Adds crash test and expectation as well. BUG=700655 Review-Url: https://codereview.chromium.org/2752163003 Cr-Commit-Position: refs/heads/master@{#462373} Committed: https://chromium.googlesource.com/chromium/src/+/6d202487ff823def4672599f51e9... ========== to ========== Clear pseudoElement from ElementRareData when FirstLetterPseudoElement doesn't have a LayoutObject This patch edits Element::rebuildPseudoElementLayoutTree() to make sure it mirrors Element::updatePseudoElement() called in Element::recaclStyle() by calling Element::updateFirstLetter() in the case the PseudoElement is firstLetter as is done in Element::updateFirstLetter() to ensure we don't call Element::retachLayoutTree() uncoditionally. Adds crash test and expectation as well. BUG=700655 Review-Url: https://codereview.chromium.org/2752163003 Cr-Original-Commit-Position: refs/heads/master@{#462373} Committed: https://chromium.googlesource.com/chromium/src/+/6d202487ff823def4672599f51e9... Review-Url: https://codereview.chromium.org/2752163003 Cr-Commit-Position: refs/heads/master@{#462705} Committed: https://chromium.googlesource.com/chromium/src/+/9e923986fcf79015b0ff237c0620... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/9e923986fcf79015b0ff237c0620... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
