|
|
Chromium Code Reviews|
Created:
4 years, 1 month ago by Navid Zolghadr Modified:
4 years, 1 month ago CC:
chromium-reviews, blink-reviews, dtapuska+blinkwatch_chromium.org, Navid Zolghadr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a null check for token creation to avoid crash
If touchpoint hit test doesn't return a node the
its targetFrame will be null and it could cause a
crash.
BUG=663608
Committed: https://crrev.com/553e31530cf9dee59135026b5644dcdef0b0d181
Cr-Commit-Position: refs/heads/master@{#431139}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add the emptiness check #Messages
Total messages: 17 (9 generated)
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org, rbyers@chromium.org
lgtm % nit https://codereview.chromium.org/2485383004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2485383004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:321: !m_inCanceledStateForPointerTypeTouch && touchInfos[0].targetFrame) { I'd feel safer if we added !touchInfo.isEmpty() as well as I wonder if this can crash if I throw an WebTouchEvent with no touch points at the renderer.
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
https://codereview.chromium.org/2485383004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2485383004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:321: !m_inCanceledStateForPointerTypeTouch && touchInfos[0].targetFrame) { On 2016/11/09 21:12:14, dtapuska wrote: > I'd feel safer if we added !touchInfo.isEmpty() as well as I wonder if this can > crash if I throw an WebTouchEvent with no touch points at the renderer. Although that would be weird to pass such a thing and I'm not sure if the rest of the code is safe to that but sure I added the check here. I actually had it in the first place and then removed it to make the change minimal to solve the crash.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/09 21:18:04, Navid Zolghadr wrote: > https://codereview.chromium.org/2485383004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > https://codereview.chromium.org/2485383004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:321: > !m_inCanceledStateForPointerTypeTouch && touchInfos[0].targetFrame) { > On 2016/11/09 21:12:14, dtapuska wrote: > > I'd feel safer if we added !touchInfo.isEmpty() as well as I wonder if this > can > > crash if I throw an WebTouchEvent with no touch points at the renderer. > > Although that would be weird to pass such a thing and I'm not sure if the rest > of the code is safe to that but sure I added the check here. I actually had it > in the first place and then removed it to make the change minimal to solve the > crash. I'd prefer if it was there. The other code I looked at iterates on the vector so it seemed safe.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Thank you! LGTM
The CQ bit was checked by dtapuska@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org Link to the patchset: https://codereview.chromium.org/2485383004/#ps20001 (title: "Add the emptiness check")
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add a null check for token creation to avoid crash If touchpoint hit test doesn't return a node the its targetFrame will be null and it could cause a crash. BUG=663608 ========== to ========== Add a null check for token creation to avoid crash If touchpoint hit test doesn't return a node the its targetFrame will be null and it could cause a crash. BUG=663608 Committed: https://crrev.com/553e31530cf9dee59135026b5644dcdef0b0d181 Cr-Commit-Position: refs/heads/master@{#431139} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/553e31530cf9dee59135026b5644dcdef0b0d181 Cr-Commit-Position: refs/heads/master@{#431139} |
