Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1972523002:
[asan][Editing] Check node->layoutObject in CaretBase::caretLayoutObject (Closed)
|
Created:
4 years, 7 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. |
DescriptionCheck node->layoutObject in CaretBase::caretLayoutObject
In CaretBase::caretLayoutObject(Node* node),
if caretRendersInsideNode(node) calls HTMLMeterElement::canContainRangeEndPoint,
it updates the layout tree. It means LayoutObject for the METER can be deleted.
This is bad design. We should make caret painting algorithm clean.
BUG=608817
Committed: https://crrev.com/e4edfb63d1b068c5ab5dc6b91edf75c108ebc433
Cr-Commit-Position: refs/heads/master@{#395822}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Change to CHECK_EQ #Messages
Total messages: 17 (7 generated)
Description was changed from ========== init608817 BUG= ========== to ========== Reassign layoutObject in CaretBase::caretLayoutObject In CaretBase::caretLayoutObject(Node* node), if caretRendersInsideNode(node) calls HTMLMeterElement::canContainRangeEndPoint, it updates the layout tree. It means LayoutObject for the METER can be deleted. Thus we should get layoutObject from node again. BUG=608817 ==========
yoichio@chromium.org changed reviewers: + yosin@google.com
yoichio@chromium.org changed reviewers: + yosin@chromium.org - yosin@google.com
https://codereview.chromium.org/1972523002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/CaretBase.cpp (right): https://codereview.chromium.org/1972523002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/CaretBase.cpp:68: layoutObject = node->layoutObject(); We can't explain what this line does. When layout tree changed, we need to re-validate selection to make selection holds canonical positions. Also, |caretLayoutObject()| should be called at least DocumentLifeCycle::LayoutClean.
On 2016/05/12 06:55:31, Yosi_UTC9 wrote: > https://codereview.chromium.org/1972523002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/CaretBase.cpp (right): > > https://codereview.chromium.org/1972523002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/CaretBase.cpp:68: layoutObject = > node->layoutObject(); > We can't explain what this line does. > When layout tree changed, we need to re-validate selection to make selection > holds canonical positions. > Also, |caretLayoutObject()| should be called at least > DocumentLifeCycle::LayoutClean. Although this is bad design, should we fix the security issue for now?
On 2016/05/24 at 04:14:23, yoichio wrote: > On 2016/05/12 06:55:31, Yosi_UTC9 wrote: > > https://codereview.chromium.org/1972523002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/editing/CaretBase.cpp (right): > > > > https://codereview.chromium.org/1972523002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/editing/CaretBase.cpp:68: layoutObject = > > node->layoutObject(); > > We can't explain what this line does. > > When layout tree changed, we need to re-validate selection to make selection > > holds canonical positions. > > Also, |caretLayoutObject()| should be called at least > > DocumentLifeCycle::LayoutClean. > > Although this is bad design, should we fix the security issue for now? No. We should do right thing == good design. The bug isn't critical and should be lower to Pri-2, Since the bug comes from Cluster Fuzz with strange usage of METER, which is rate in real-world.
https://codereview.chromium.org/1972523002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/CaretBase.cpp (right): https://codereview.chromium.org/1972523002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/CaretBase.cpp:68: layoutObject = node->layoutObject(); On 2016/05/12 at 06:55:31, Yosi_UTC9 wrote: > We can't explain what this line does. > When layout tree changed, we need to re-validate selection to make selection holds canonical positions. > Also, |caretLayoutObject()| should be called at least DocumentLifeCycle::LayoutClean. Could you change this to below? // TODO(yoichio): blah blah blah CHECK_EQ(layoutObject, node->layoutObject()) << "Layout tree should not changed";
Description was changed from ========== Reassign layoutObject in CaretBase::caretLayoutObject In CaretBase::caretLayoutObject(Node* node), if caretRendersInsideNode(node) calls HTMLMeterElement::canContainRangeEndPoint, it updates the layout tree. It means LayoutObject for the METER can be deleted. Thus we should get layoutObject from node again. BUG=608817 ========== to ========== Check node->layoutObject in CaretBase::caretLayoutObject In CaretBase::caretLayoutObject(Node* node), if caretRendersInsideNode(node) calls HTMLMeterElement::canContainRangeEndPoint, it updates the layout tree. It means LayoutObject for the METER can be deleted. This is bad design. We should make caret painting algorithm clean. BUG=608817 ==========
https://codereview.chromium.org/1972523002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/CaretBase.cpp (right): https://codereview.chromium.org/1972523002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/CaretBase.cpp:68: layoutObject = node->layoutObject(); On 2016/05/24 08:40:18, Yosi_UTC9 wrote: > On 2016/05/12 at 06:55:31, Yosi_UTC9 wrote: > > We can't explain what this line does. > > When layout tree changed, we need to re-validate selection to make selection > holds canonical positions. > > Also, |caretLayoutObject()| should be called at least > DocumentLifeCycle::LayoutClean. > > Could you change this to below? > // TODO(yoichio): blah blah blah > CHECK_EQ(layoutObject, node->layoutObject()) << "Layout tree should not > changed"; Done.
The CQ bit was checked by yosin@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1972523002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1972523002/20001
Message was sent while issue was closed.
Description was changed from ========== Check node->layoutObject in CaretBase::caretLayoutObject In CaretBase::caretLayoutObject(Node* node), if caretRendersInsideNode(node) calls HTMLMeterElement::canContainRangeEndPoint, it updates the layout tree. It means LayoutObject for the METER can be deleted. This is bad design. We should make caret painting algorithm clean. BUG=608817 ========== to ========== Check node->layoutObject in CaretBase::caretLayoutObject In CaretBase::caretLayoutObject(Node* node), if caretRendersInsideNode(node) calls HTMLMeterElement::canContainRangeEndPoint, it updates the layout tree. It means LayoutObject for the METER can be deleted. This is bad design. We should make caret painting algorithm clean. BUG=608817 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Check node->layoutObject in CaretBase::caretLayoutObject In CaretBase::caretLayoutObject(Node* node), if caretRendersInsideNode(node) calls HTMLMeterElement::canContainRangeEndPoint, it updates the layout tree. It means LayoutObject for the METER can be deleted. This is bad design. We should make caret painting algorithm clean. BUG=608817 ========== to ========== Check node->layoutObject in CaretBase::caretLayoutObject In CaretBase::caretLayoutObject(Node* node), if caretRendersInsideNode(node) calls HTMLMeterElement::canContainRangeEndPoint, it updates the layout tree. It means LayoutObject for the METER can be deleted. This is bad design. We should make caret painting algorithm clean. BUG=608817 Committed: https://crrev.com/e4edfb63d1b068c5ab5dc6b91edf75c108ebc433 Cr-Commit-Position: refs/heads/master@{#395822} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e4edfb63d1b068c5ab5dc6b91edf75c108ebc433 Cr-Commit-Position: refs/heads/master@{#395822} |
