Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2636623002:
Fix crash in localSelectionRectOfPositionTemplate() in VisibleUnits.cpp (Closed)
|
Created:
3 years, 11 months ago by joone Modified:
3 years, 11 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews, yoichio Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix crash in localSelectionRectOfPositionTemplate() in VisibleUnits.cpp.
It only happens during scrolling in Android.
BUG=680428
Patch Set 1 #Patch Set 2 : return empty rect #Patch Set 3 : add a test #
Total comments: 4
Patch Set 4 : call absoluteCaretBoundsOf #Patch Set 5 : same as the patch set 2 #Patch Set 6 : embrace localCaretRectOfPositionTemplate() #
Total comments: 3
Messages
Total messages: 44 (28 generated)
The CQ bit was checked by joone.hur@intel.com 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...
Description was changed from ========== Fix crash in localSelectionRectOfPositionTemplate() in VisibleUnits.cpp BUG=680428 ========== to ========== Fix crash in localSelectionRectOfPositionTemplate() in VisibleUnits.cpp. It only happens in Android. BUG=680428 ==========
joone.hur@intel.com changed reviewers: + yosin@chromium.org
Please review this CL.
The CQ bit was checked by joone.hur@intel.com 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.
Could you add a test case for this[1]? gTest is better. [1] Importance and requirements of testing in Chromium; https://goo.gl/OpRI6x
On 2017/01/16 02:25:37, Yosi_UTC9 wrote: > Could you add a test case for this[1]? gTest is better. > > [1] Importance and requirements of testing in Chromium; https://goo.gl/OpRI6x Do you want me to add a test for the below APIs I added? CORE_EXPORT IntRect absoluteSelectionBoundsOf(const VisiblePosition&); CORE_EXPORT IntRect absoluteSelectionBoundsOf(const VisiblePositionInFlatTree&);
Description was changed from ========== Fix crash in localSelectionRectOfPositionTemplate() in VisibleUnits.cpp. It only happens in Android. BUG=680428 ========== to ========== Fix crash in localSelectionRectOfPositionTemplate() in VisibleUnits.cpp. It only happens in Android. BUG=680428 TEST=VisibleUnitsTest.absoluteSelectionBoundsOf ==========
The CQ bit was checked by joone.hur@intel.com 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.
On 2017/01/17 at 05:50:37, joone.hur wrote: > On 2017/01/16 02:25:37, Yosi_UTC9 wrote: > > Could you add a test case for this[1]? gTest is better. > > > > [1] Importance and requirements of testing in Chromium; https://goo.gl/OpRI6x > > Do you want me to add a test for the below APIs I added? > CORE_EXPORT IntRect absoluteSelectionBoundsOf(const VisiblePosition&); > CORE_EXPORT IntRect absoluteSelectionBoundsOf(const VisiblePositionInFlatTree&); No. This patch should change localSelectionRectOfPosition() and having test case for localSelectionRectOfPosition(). But, it seems root cause of this crash is its call sites. We've never seen this so far. Attached test case is something make sense in point of DOM tree version and flat tree version return same caret bounds. But it doesn't represent why we have nullptr on localSelectionRectOfPosition(). Regarding #c7 of issue 680428, it seems scrolling cause this. We've not had layout object of them? Call site is: VisiblePosition oldOffsetExtentPosition = selection.visibleExtent(); IntPoint oldExtentLocation = positionLocation(oldOffsetExtentPosition); positionLocation(vp) = absoluteSelectionBoundsOf(vp).minXMaxYCorner();
https://codereview.chromium.org/2636623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2636623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2604: LayoutRect localSelectionRectOfPosition( This function doesn't relate to change of this patch. Please get rid of this. https://codereview.chromium.org/2636623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:3301: IntRect absoluteSelectionBoundsOf( This function doesn't relate to change of this patch. Please get rid of this.
On 2017/01/18 01:56:09, Yosi_UTC9 wrote: > On 2017/01/17 at 05:50:37, joone.hur wrote: > > On 2017/01/16 02:25:37, Yosi_UTC9 wrote: > > > Could you add a test case for this[1]? gTest is better. > > > > > > [1] Importance and requirements of testing in Chromium; > https://goo.gl/OpRI6x > > > > Do you want me to add a test for the below APIs I added? > > CORE_EXPORT IntRect absoluteSelectionBoundsOf(const VisiblePosition&); > > CORE_EXPORT IntRect absoluteSelectionBoundsOf(const > VisiblePositionInFlatTree&); > > No. This patch should change localSelectionRectOfPosition() and having test case > for > localSelectionRectOfPosition(). > How can I change localSelectionRectOfPosition? Instead, I changed localSelectionRectOfPositionTemplate. > But, it seems root cause of this crash is its call sites. We've never seen this > so far. > Attached test case is something make sense in point of DOM tree version and flat > tree > version return same caret bounds. But it doesn't represent why we have nullptr > on > localSelectionRectOfPosition(). > I will create another CL for this API test. > Regarding #c7 of issue 680428, it seems scrolling cause this. We've not had > layout > object of them? > > Call site is: > > VisiblePosition oldOffsetExtentPosition = selection.visibleExtent(); > IntPoint oldExtentLocation = positionLocation(oldOffsetExtentPosition); > > positionLocation(vp) = absoluteSelectionBoundsOf(vp).minXMaxYCorner();
https://codereview.chromium.org/2636623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2636623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2604: LayoutRect localSelectionRectOfPosition( On 2017/01/18 05:37:50, Yosi_UTC9 wrote: > This function doesn't relate to change of this patch. Please get rid of this. This function should have been added to the previous CL so I will separate this change. https://codereview.chromium.org/2636623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:3301: IntRect absoluteSelectionBoundsOf( On 2017/01/18 05:37:50, Yosi_UTC9 wrote: > This function doesn't relate to change of this patch. Please get rid of this. Ditto.
On 2017/01/18 07:52:17, joone wrote: > https://codereview.chromium.org/2636623002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): > > https://codereview.chromium.org/2636623002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2604: LayoutRect > localSelectionRectOfPosition( > On 2017/01/18 05:37:50, Yosi_UTC9 wrote: > > This function doesn't relate to change of this patch. Please get rid of this. > > This function should have been added to the previous CL so I will separate this > change. > > https://codereview.chromium.org/2636623002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/VisibleUnits.cpp:3301: IntRect > absoluteSelectionBoundsOf( > On 2017/01/18 05:37:50, Yosi_UTC9 wrote: > > This function doesn't relate to change of this patch. Please get rid of this. > > Ditto. We need to land this patch *today* or we need to revert the root cause from trunk, we cannot continue to generate builds from trunk with this crash present.
The CQ bit was checked by joone.hur@intel.com 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...
On 2017/01/18 01:56:09, Yosi_UTC9 wrote: > > Call site is: > > VisiblePosition oldOffsetExtentPosition = selection.visibleExtent(); > IntPoint oldExtentLocation = positionLocation(oldOffsetExtentPosition); > > positionLocation(vp) = absoluteSelectionBoundsOf(vp).minXMaxYCorner(); We had better call absoluteCaretBoundsOf(vp).minXMaxYCorner() instead of absoluteSelectionBoundsOf(vp).minXMaxYCorner() in order to get the position location that is used by scrolling.
On 2017/01/18 16:16:53, amineer wrote: > On 2017/01/18 07:52:17, joone wrote: > > > https://codereview.chromium.org/2636623002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): > > > > > https://codereview.chromium.org/2636623002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2604: LayoutRect > > localSelectionRectOfPosition( > > On 2017/01/18 05:37:50, Yosi_UTC9 wrote: > > > This function doesn't relate to change of this patch. Please get rid of > this. > > > > This function should have been added to the previous CL so I will separate > this > > change. > > > > > https://codereview.chromium.org/2636623002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/VisibleUnits.cpp:3301: IntRect > > absoluteSelectionBoundsOf( > > On 2017/01/18 05:37:50, Yosi_UTC9 wrote: > > > This function doesn't relate to change of this patch. Please get rid of > this. > > > > Ditto. > > We need to land this patch *today* or we need to revert the root cause from > trunk, we cannot continue to generate builds from trunk with this crash present. amineer@ I've updated the CL. Could you test this CL?
Description was changed from ========== Fix crash in localSelectionRectOfPositionTemplate() in VisibleUnits.cpp. It only happens in Android. BUG=680428 TEST=VisibleUnitsTest.absoluteSelectionBoundsOf ========== to ========== Fix crash in localSelectionRectOfPositionTemplate() in VisibleUnits.cpp. It only happens during scrolling in Android so we need to run absoluteCaretBoundsOf() instead of absoluteSelectionBoundsOf() in order to get the position location. BUG=680428 TEST=VisibleUnitsTest.absoluteSelectionBoundsOf ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by joone.hur@intel.com 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...
Description was changed from ========== Fix crash in localSelectionRectOfPositionTemplate() in VisibleUnits.cpp. It only happens during scrolling in Android so we need to run absoluteCaretBoundsOf() instead of absoluteSelectionBoundsOf() in order to get the position location. BUG=680428 TEST=VisibleUnitsTest.absoluteSelectionBoundsOf ========== to ========== Fix crash in localSelectionRectOfPositionTemplate() in VisibleUnits.cpp. It only happens during scrolling in Android. BUG=680428 TEST=VisibleUnitsTest.absoluteSelectionBoundsOf ==========
yosin@ it will take more time to write a test case for this crash problem because it is hard to reproduce the crash in gTest.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/18 at 22:25:31, joone.hur wrote: > yosin@ it will take more time to write a test case for this crash problem because it is hard to reproduce the crash in gTest. Do you have a sample HTML to reproduce this? I think we should have a nullptr here and last 10+ years we don't have it. We want to know what HTML causes this.
On 2017/01/19 01:19:55, Yosi_UTC9 wrote: > On 2017/01/18 at 22:25:31, joone.hur wrote: > > yosin@ it will take more time to write a test case for this crash problem > because it is hard to reproduce the crash in gTest. > > Do you have a sample HTML to reproduce this? > I think we should have a nullptr here and last 10+ years we don't have it. We > want to know what HTML causes this. No, I need to write one, but I was wondering why this crash is only happening in Android.
On 2017/01/19 at 01:59:56, joone.hur wrote: > On 2017/01/19 01:19:55, Yosi_UTC9 wrote: > > On 2017/01/18 at 22:25:31, joone.hur wrote: > > > yosin@ it will take more time to write a test case for this crash problem > > because it is hard to reproduce the crash in gTest. > > > > Do you have a sample HTML to reproduce this? > > I think we should have a nullptr here and last 10+ years we don't have it. We > > want to know what HTML causes this. > > No, I need to write one, but I was wondering why this crash is only happening in Android. We'are also investigating this crash. We can reproduce this in local Android. It seems scrolling causes this, but not sure.
Description was changed from ========== Fix crash in localSelectionRectOfPositionTemplate() in VisibleUnits.cpp. It only happens during scrolling in Android. BUG=680428 TEST=VisibleUnitsTest.absoluteSelectionBoundsOf ========== to ========== Fix crash in localSelectionRectOfPositionTemplate() in VisibleUnits.cpp. It only happens during scrolling in Android. BUG=680428 ==========
The CQ bit was checked by joone.hur@intel.com 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...
yosin@ please review the updated CL. I tested it with Nexus5 . https://codereview.chromium.org/2636623002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2636623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2584: if (rect.isEmpty()) If boxPosition.inlineBox isn't InlineTextBox, localCaretRect returns empty rect.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think caret height fix has big benefit to users. Let's try to find out test case and fix absoluteSelectionRectOf() until time expired, Friday 5PM PST. https://codereview.chromium.org/2636623002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2636623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2584: if (rect.isEmpty()) On 2017/01/20 at 02:07:29, joone wrote: > If boxPosition.inlineBox isn't InlineTextBox, localCaretRect returns empty rect. We need to have a test case of this. I think BR or IMG may be case. We also attempt to try to find test case. Let's try to seek test case rather than reverting patch as time allows. ;-) https://codereview.chromium.org/2636623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2587: InlineTextBox* box = toInlineTextBox(boxPosition.inlineBox); We need to handle if |boxPosition.inlineBox| isn't InlineTextBox. We also need to have a test case of this. |
