Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1793093006:
[Editing][Stability] Recanonicalize m_originalbase in FrameSelection (Closed)
|
Created:
4 years, 9 months ago by yoichio Modified:
4 years, 7 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Editing][Stability] Recanonicalize m_originalbase in FrameSelection
The crash issue 562339 is caused in computeInlineBoxPositionTemplate called in the
RenderPosition constructor.
The root issue is FrameSelection::setNonDirectionalSelectionIfNeededAlgorithm
uses m_originalBase as up-to-date.
m_originBase is used only in the function.
However, there can be layout by next call.
Thus this CL recanonicalizes m_originalBase.
BUG=562339, 593592
Committed: https://crrev.com/1c6aad352c275e7965fb4f32cebcdf5b261af704
Cr-Commit-Position: refs/heads/master@{#393483}
Patch Set 1 #
Total comments: 1
Patch Set 2 : update #
Total comments: 10
Patch Set 3 : update layout test #
Total comments: 6
Patch Set 4 : update test #
Total comments: 4
Patch Set 5 : nit pick #
Messages
Total messages: 23 (10 generated)
Description was changed from ========== is562339 ccrash BUG= ========== to ========== Let computeInlineBoxPositionTemplate return null InlineBoxPosition The crash issue 562339 is caused in computeInlineBoxPositionTemplate called in the RenderPosition constructor. The root issue is computeInlineBoxPositionTemplate doesn't check if anochor node's layout object is null. computeInlineBoxPositionTemplate can return null InlineBoxPosition(see L1942) so this change is safe. BUG=562339 ==========
yoichio@chromium.org changed reviewers: + yosin@chromium.org
https://codereview.chromium.org/1793093006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/1793093006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1930: if (!layoutObject) We should assume |computeInlineBoxPosition()| called with valid position, e.g. inDocument() and associated to layout object. It seems root cause is |FrameSelection| holds obsoleted positions in |m_originalBase| and |m_originalBaseInFlatTree|, which is passed to |adjustEndpointsAtBidiBoundar()| then |RenderedPosition| ctor. Since we don't update |m_originalBase| in |FrameSelection::nodeWillBeRemove()| and |setOriginalBase()| updates only one of DOM tree or Flat tree original base position. |setOriginalBase()| should update both DOM ree and Flat tree position.
Description was changed from ========== Let computeInlineBoxPositionTemplate return null InlineBoxPosition The crash issue 562339 is caused in computeInlineBoxPositionTemplate called in the RenderPosition constructor. The root issue is computeInlineBoxPositionTemplate doesn't check if anochor node's layout object is null. computeInlineBoxPositionTemplate can return null InlineBoxPosition(see L1942) so this change is safe. BUG=562339 ========== to ========== Let computeInlineBoxPositionTemplate return null InlineBoxPosition The crash issue 562339 is caused in computeInlineBoxPositionTemplate called in the RenderPosition constructor. The root issue is computeInlineBoxPositionTemplate doesn't check if anochor node's layout object is null. computeInlineBoxPositionTemplate can return null InlineBoxPosition(see L1942) so this change is safe. BUG=562339,593592 ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Description was changed from ========== Let computeInlineBoxPositionTemplate return null InlineBoxPosition The crash issue 562339 is caused in computeInlineBoxPositionTemplate called in the RenderPosition constructor. The root issue is computeInlineBoxPositionTemplate doesn't check if anochor node's layout object is null. computeInlineBoxPositionTemplate can return null InlineBoxPosition(see L1942) so this change is safe. BUG=562339,593592 ========== to ========== [Editing][Stability] Recanonicalize m_originalbase in FrameSelection The crash issue 562339 is caused in computeInlineBoxPositionTemplate called in the RenderPosition constructor. The root issue is FrameSelection::setNonDirectionalSelectionIfNeededAlgorithm uses m_originalBase as up-to-date. m_originBase is used only in the function. However, there can be layout by next call. Thus this CL recanonicalizes m_originalBase. BUG=562339,593592 ==========
On 2016/03/15 06:20:41, Yosi_UTC9 wrote: > https://codereview.chromium.org/1793093006/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): > > https://codereview.chromium.org/1793093006/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1930: if (!layoutObject) > We should assume |computeInlineBoxPosition()| called with valid position, e.g. > inDocument() and associated to layout object. > > It seems root cause is |FrameSelection| holds obsoleted positions in > |m_originalBase| and |m_originalBaseInFlatTree|, which is passed to > |adjustEndpointsAtBidiBoundar()| then |RenderedPosition| ctor. Since we don't > update |m_originalBase| in |FrameSelection::nodeWillBeRemove()| and > |setOriginalBase()| updates only one of DOM tree or Flat tree original base > position. |setOriginalBase()| should update both DOM ree and Flat tree position. Done. PTAL.
C++ changes are fine. https://codereview.chromium.org/1793093006/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html (right): https://codereview.chromium.org/1793093006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:5: var __v_40 = 0; ++__v_40 {__v_40: "ab" + __v_40 + (__v_40 / 100000);.target { olor: #bbeeff; Can we replace this cryptic text by "foo" or simple text? https://codereview.chromium.org/1793093006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:8: abcאבג nit: Could you use אבג to make visible in poor console? https://codereview.chromium.org/1793093006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:21: for (var x = startX; left <= x && x <= left + target.offsetWidth; x += xIncrement) { Just one |moveMoveTo()| should be enough. https://codereview.chromium.org/1793093006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:22: eventSender.mouseMoveTo(x, y); nit: indent https://codereview.chromium.org/1793093006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:25: document.body.innerHTML = 'text'; I think we can have "<div id=log></div>", once we can, we don't need to clear document.body.
https://codereview.chromium.org/1793093006/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html (right): https://codereview.chromium.org/1793093006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:5: var __v_40 = 0; ++__v_40 {__v_40: "ab" + __v_40 + (__v_40 / 100000);.target { olor: #bbeeff; On 2016/04/19 06:04:04, Yosi_UTC9 wrote: > Can we replace this cryptic text by "foo" or simple text? This test depends on this strange text lined through RTL. https://codereview.chromium.org/1793093006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:8: abcאבג On 2016/04/19 06:04:04, Yosi_UTC9 wrote: > nit: Could you use אבג to make visible in poor console? Done. https://codereview.chromium.org/1793093006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:21: for (var x = startX; left <= x && x <= left + target.offsetWidth; x += xIncrement) { On 2016/04/19 06:04:04, Yosi_UTC9 wrote: > Just one |moveMoveTo()| should be enough. Done. https://codereview.chromium.org/1793093006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:22: eventSender.mouseMoveTo(x, y); On 2016/04/19 06:04:04, Yosi_UTC9 wrote: > nit: indent Done. https://codereview.chromium.org/1793093006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:25: document.body.innerHTML = 'text'; On 2016/04/19 06:04:04, Yosi_UTC9 wrote: > I think we can have "<div id=log></div>", once we can, we don't need to clear > document.body. Done.
https://codereview.chromium.org/1793093006/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html (right): https://codereview.chromium.org/1793093006/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:4: <div style="direction: rtl;"> <div dir=rtl> https://codereview.chromium.org/1793093006/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:5: __v_40 = 0; ++__v_40 {__v_40: "ab" + __v_40 + (__v_40 / 100000);.target { olor: #; The issue is caused by selecting text in RTL. zoom factor and this cryptic text should not be affected. https://codereview.chromium.org/1793093006/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:20: document.body.innerHTML = '<div id="log"></div>'; Can we put this into HTML? After <div dir=rtl">?
https://codereview.chromium.org/1793093006/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html (right): https://codereview.chromium.org/1793093006/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:4: <div style="direction: rtl;"> On 2016/04/19 09:41:20, Yosi_UTC9 wrote: > <div dir=rtl> Acknowledged. https://codereview.chromium.org/1793093006/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:5: __v_40 = 0; ++__v_40 {__v_40: "ab" + __v_40 + (__v_40 / 100000);.target { olor: #; On 2016/04/19 09:41:20, Yosi_UTC9 wrote: > The issue is caused by selecting text in RTL. zoom factor and this cryptic text > should not be affected. Done. https://codereview.chromium.org/1793093006/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:20: document.body.innerHTML = '<div id="log"></div>'; On 2016/04/19 09:41:20, Yosi_UTC9 wrote: > Can we put this into HTML? After <div dir=rtl">? Done.
lgtm w/ small nits https://codereview.chromium.org/1793093006/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html (right): https://codereview.chromium.org/1793093006/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:8: var y = target.offsetTop + target.offsetHeight / 2; Please notify this test script requires |eventSender|, e.g. if (!eventSender) throw new Error('eventSender is required'); https://codereview.chromium.org/1793093006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/1793093006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1929: DCHECK(layoutObject); Since L1931, deref |layoutObject|, we don't need to have this DCHECK(). But, for ease of debugging, how about DCHECK(layoutObject) << position;
https://codereview.chromium.org/1793093006/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html (right): https://codereview.chromium.org/1793093006/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_selection_update_crash.html:8: var y = target.offsetTop + target.offsetHeight / 2; On 2016/05/13 05:09:21, Yosi_UTC9 wrote: > Please notify this test script requires |eventSender|, e.g. > if (!eventSender) > throw new Error('eventSender is required'); Done. https://codereview.chromium.org/1793093006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/1793093006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1929: DCHECK(layoutObject); On 2016/05/13 05:09:21, Yosi_UTC9 wrote: > Since L1931, deref |layoutObject|, we don't need to have this DCHECK(). > But, for ease of debugging, how about > DCHECK(layoutObject) << position; Done.
The CQ bit was checked by yoichio@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/1793093006/#ps120001 (title: "nit pick")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1793093006/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1793093006/120001
Message was sent while issue was closed.
Description was changed from ========== [Editing][Stability] Recanonicalize m_originalbase in FrameSelection The crash issue 562339 is caused in computeInlineBoxPositionTemplate called in the RenderPosition constructor. The root issue is FrameSelection::setNonDirectionalSelectionIfNeededAlgorithm uses m_originalBase as up-to-date. m_originBase is used only in the function. However, there can be layout by next call. Thus this CL recanonicalizes m_originalBase. BUG=562339,593592 ========== to ========== [Editing][Stability] Recanonicalize m_originalbase in FrameSelection The crash issue 562339 is caused in computeInlineBoxPositionTemplate called in the RenderPosition constructor. The root issue is FrameSelection::setNonDirectionalSelectionIfNeededAlgorithm uses m_originalBase as up-to-date. m_originBase is used only in the function. However, there can be layout by next call. Thus this CL recanonicalizes m_originalBase. BUG=562339,593592 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Editing][Stability] Recanonicalize m_originalbase in FrameSelection The crash issue 562339 is caused in computeInlineBoxPositionTemplate called in the RenderPosition constructor. The root issue is FrameSelection::setNonDirectionalSelectionIfNeededAlgorithm uses m_originalBase as up-to-date. m_originBase is used only in the function. However, there can be layout by next call. Thus this CL recanonicalizes m_originalBase. BUG=562339,593592 ========== to ========== [Editing][Stability] Recanonicalize m_originalbase in FrameSelection The crash issue 562339 is caused in computeInlineBoxPositionTemplate called in the RenderPosition constructor. The root issue is FrameSelection::setNonDirectionalSelectionIfNeededAlgorithm uses m_originalBase as up-to-date. m_originBase is used only in the function. However, there can be layout by next call. Thus this CL recanonicalizes m_originalBase. BUG=562339,593592 Committed: https://crrev.com/1c6aad352c275e7965fb4f32cebcdf5b261af704 Cr-Commit-Position: refs/heads/master@{#393483} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1c6aad352c275e7965fb4f32cebcdf5b261af704 Cr-Commit-Position: refs/heads/master@{#393483}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:120001) has been created in https://codereview.chromium.org/1971393003/ by vasilii@chromium.org. The reason for reverting is: Causes unexpected leak reliably https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b... 02:43:31.446 20471 worker/6 editing/selection/mouse/drag_selection_update_crash.html leaked 02:43:31.447 20404 [15652/40740] editing/selection/mouse/drag_selection_update_crash.html failed unexpectedly (leak detected: ({"numberOfLiveActiveDOMObjects":[2,4],"numberOfLiveDocuments":[1,2],"numberOfLiveNodes":[4,18],"numberOfLiveResources":[0,2]})) 02:43:31.446 20471 worker/6 editing/selection/mouse/drag_selection_update_crash.html failed: 02:43:31.446 20471 worker/6 leak detected: ({"numberOfLiveActiveDOMObjects":[2,4],"numberOfLiveDocuments":[1,2],"numberOfLiveNodes":[4,18],"numberOfLiveResources":[0,2]}). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
