New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 752648 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Google Japanese Input IME is crashing on DCHECK in debug builds of master

Project Member Reported by rlanday@chromium.org, Aug 4 2017

Issue description

Chrome Version: 62.0.3188.0 (Developer Build) unknown (32-bit)
OS: Android N

What steps will reproduce the problem?
(1) Install the Google Japanese Input IME
(2) Go to editpad.org
(3) Try typing with the Japanese IME

What is the expected result?

Japanese characters appear underlined with a blue background.

What happens instead?

Chrome crashes on this DCHECK to verify that we're not adding overlapping composition DocumentMarkers:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp?q=documentmarkerlisteditor&sq=package:chromium&l=29

In release builds, we do not crash on the DCHECK, but buggy behavior may be occurring since an invariant we thought was true about composition markers (they do not overlap) is being violated.
 
It looks like the IME is adding two CompositionUnderlines on the same span of text: one for the underline and one for the background color. Should we change CompositionMarkerListImpl to support overlapping composition markers?

Comment 2 by yosin@chromium.org, Aug 7 2017

Can we ask IME team to call one set marker to specify both underline and background color to reduce IPC?

I don't think this is possible. Android IMEs are built on top of the Android APIs, so they have to use BackgroundColorSpan and UnderlineSpan together if they want to set both (if there's a way to do both together, I'm not aware of it). In this particular case, where they apply to the same range, we could merge them in ImeAdapterAndroid when we convert them to CompositionUnderlines. But in general, an IME can apply multiple BackgroundColorSpans and UnderlineSpans to different regions in a way that can't be consolidated into a single marker. So I think it makes sense to support the general case.

I don't think asking the authors of every single IME that uses these features to change their behavior for our benefit would be reasonable anyway. I think our strategy is generally to implement the APIs (at least, those actually used by IMEs) the same way the Android OS does.
I think the right solution might be to do something like:

- Rename DocumentMarker::kComposition to kImeFormattingSpan
- Modify CompositionMarkerListImpl to support overlapping markers (and rename to ImeFormattingMarkerListImpl)
- Rename CompositionUnderline (e.g. to IMESpan if we also want to use it for suggestion markers, or IMEFormattingSpan if not)

Then InputMethodController can still keep track of the actual composition range in composition_range_, and it will be clear that the DocumentMarkers are just used for formatting.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 28 2017

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

commit 1b051d44ad4685de6d32a65f554d0a42bc0e6170
Author: Ryan Landay <rlanday@chromium.org>
Date: Mon Aug 28 19:59:59 2017

Rename DocumentMarkerListEditor to SortedDocumentMarkerListEditor

I'm going to introduce a new class, UnsortedDocumentMarkerListEditor, for marker
types that we store unsorted because they can overlap. Currently, suggestion
markers are the only markers we have of this type, so the logic is implemented
directly in SuggestionMarkerListImpl, but I also want to make
CompositionMarkerListImpl use the same logic to fix  crbug.com/752648 . Therefore,
we need to rename DocumentMarkerListEditor to clarify that it should only be
used for MarkerListImpls that store markers in sorted order.

Bug:  752648 
Change-Id: I796e8d23cb75a3c45e6c1639ad67bc266668910f
Reviewed-on: https://chromium-review.googlesource.com/636181
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Ryan Landay <rlanday@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497854}
[modify] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/BUILD.gn
[modify] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/ActiveSuggestionMarkerListImpl.cpp
[modify] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp
[modify] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
[delete] https://crrev.com/5302e35e0230a5661f081732b621241cfa00c4bf/third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditorTest.cpp
[rename] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/SortedDocumentMarkerListEditor.cpp
[rename] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/SortedDocumentMarkerListEditor.h
[add] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/SortedDocumentMarkerListEditorTest.cpp
[modify] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImpl.cpp
[modify] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/SuggestionMarkerListImpl.cpp
[modify] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListImpl.cpp

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 29 2017

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

commit 54f103ed83effdc51456daa7212929f607b9e04d
Author: Ryan Landay <rlanday@chromium.org>
Date: Tue Aug 29 23:55:20 2017

Introduce UnsortedDocumentMarkerListEditor, part 1

This CL spins off some of SuggestionMarkerListImpl's method implementations into
a new utility class, UnsortedDocumentMarkerListEditor, so we can reuse them in
CompositionMarkerListImpl (which currently stores its markers in sorted order,
but I want to change it to allow overlapping markers and store them in unsorted
order to fix  crbug.com/752648 ).

Bug:  752648 
Change-Id: Ia917784d3685f72e0906e77d7a57ba65199f1781
Reviewed-on: https://chromium-review.googlesource.com/636843
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Ryan Landay <rlanday@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498291}
[modify] https://crrev.com/54f103ed83effdc51456daa7212929f607b9e04d/third_party/WebKit/Source/core/editing/markers/SuggestionMarkerListImpl.cpp
[modify] https://crrev.com/54f103ed83effdc51456daa7212929f607b9e04d/third_party/WebKit/Source/core/editing/markers/SuggestionMarkerListImplTest.cpp
[add] https://crrev.com/54f103ed83effdc51456daa7212929f607b9e04d/third_party/WebKit/Source/core/editing/markers/UnsortedDocumentMarkerListEditor.h

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 30 2017

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

commit cb2f855cbc7b82e20387eaf9a43f6b99b6105061
Author: Ryan Landay <rlanday@chromium.org>
Date: Wed Aug 30 18:15:48 2017

Introduce UnsortedDocumentMarkerListEditor, part 2

In a previous CL, I moved some method implementations from
SuggestionMarkerListImpl into a new class, UnsortedDocumentMarkerListEditor:
https://chromium-review.googlesource.com/c/chromium/src/+/636843

In that CL, I left these method implementations in SuggestionMarkerListImpl.cpp
to make the CL smaller. I also changed some test cases in
SuggestionMarkerListImplTest to test UnsortedDocumentMarkerListEditor directly,
and left those in SuggestionMarkerListImplTest.cpp. In this CL, I am moving
these methods to UnsortedDocumentMarkerListEditor.cpp and
UnsortedDocumentMarkerListEditorTest.cpp, respectively.

Bug:  752648 
Change-Id: Ib260fdde81c0637dfec29a5fdd786cb399e6b86f
Reviewed-on: https://chromium-review.googlesource.com/639317
Commit-Queue: Ryan Landay <rlanday@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498537}
[modify] https://crrev.com/cb2f855cbc7b82e20387eaf9a43f6b99b6105061/third_party/WebKit/Source/core/editing/BUILD.gn
[modify] https://crrev.com/cb2f855cbc7b82e20387eaf9a43f6b99b6105061/third_party/WebKit/Source/core/editing/markers/SuggestionMarkerListImpl.cpp
[modify] https://crrev.com/cb2f855cbc7b82e20387eaf9a43f6b99b6105061/third_party/WebKit/Source/core/editing/markers/SuggestionMarkerListImplTest.cpp
[add] https://crrev.com/cb2f855cbc7b82e20387eaf9a43f6b99b6105061/third_party/WebKit/Source/core/editing/markers/UnsortedDocumentMarkerListEditor.cpp
[add] https://crrev.com/cb2f855cbc7b82e20387eaf9a43f6b99b6105061/third_party/WebKit/Source/core/editing/markers/UnsortedDocumentMarkerListEditorTest.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 30 2017

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

commit 5916acd85b148e534152b329ecc682bdcb7cd427
Author: Ryan Landay <rlanday@chromium.org>
Date: Wed Aug 30 22:17:33 2017

Add UnsortedDocumentMarkerListEditor::ShiftMarkersContentIndependent()

This method is not used by SuggestionMarkerListImpl, and therefore was not
included in the original CLs to pull out SuggestionMarkerListImpl's methods into
UnsortedDocumentMarkerListEditor. However, we do need it to implement
CompositionMarkerListImpl on top of UnsortedDocumentMarkerListEditor, so I'm
adding it here.

Bug:  752648 
Change-Id: Ibd006cdfb94a0060a9fc2c0962b92be9426e6528
Reviewed-on: https://chromium-review.googlesource.com/644145
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Ryan Landay <rlanday@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498638}
[modify] https://crrev.com/5916acd85b148e534152b329ecc682bdcb7cd427/third_party/WebKit/Source/core/editing/markers/UnsortedDocumentMarkerListEditor.cpp
[modify] https://crrev.com/5916acd85b148e534152b329ecc682bdcb7cd427/third_party/WebKit/Source/core/editing/markers/UnsortedDocumentMarkerListEditorTest.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 30 2017

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

commit d36a47b56a1f0ea317f51de5124aa30d35cb6633
Author: Ryan Landay <rlanday@chromium.org>
Date: Wed Aug 30 22:26:37 2017

Add support for overlapping markers in CompositionMarkerListImpl

This CL changes CompositionMarkerListImpl from storing markers in sorted order
(using methods from SortedDocumentMarkerListEditor) to storing markers in
unsorted order (using methods from UnsortedDocumentMarkerListEditor), so we can
officially support overlapping composition markers, as e.g. the Japanese IME is
already creating.

Bug:  752648 
Change-Id: Ibf79a252eb891f08e9090056d241b87df3514d57
Reviewed-on: https://chromium-review.googlesource.com/644207
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Ryan Landay <rlanday@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498643}
[modify] https://crrev.com/d36a47b56a1f0ea317f51de5124aa30d35cb6633/third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp
[modify] https://crrev.com/d36a47b56a1f0ea317f51de5124aa30d35cb6633/third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.h
[modify] https://crrev.com/d36a47b56a1f0ea317f51de5124aa30d35cb6633/third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImplTest.cpp

Labels: M-62
Status: Fixed (was: Assigned)

Sign in to add a comment