Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2473293002:
Naive fix of a call on a nil object in CompositedLayerMapping::containingSquashedLayer. (Closed)
|
Created:
4 years, 1 month ago by krasin1 Modified:
4 years, 1 month ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNaive fix of a call on a nil object in CompositedLayerMapping::containingSquashedLayer.
layoutObject happens to be NULL in some cases. This CL fixes an undefined behavior by
avoiding making calls on a null object.
BUG=662265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/a0c9881a5470c0af5ba0faefb785e6c0751c669e
Cr-Commit-Position: refs/heads/master@{#430024}
Patch Set 1 #Patch Set 2 : Naive fix of a call on a nil object in CompositedLayerMapping::containingSquashedLayer. #Messages
Total messages: 25 (11 generated)
Description was changed from ========== Naive fix of a call on a nil object in CompositedLayerMapping::containingSquashedLayer. layoutObject happens to be NULL in some cases. This CL fixes an undedefined behavior by avoiding making calls on a null object. BUG=662265 ========== to ========== Naive fix of a call on a nil object in CompositedLayerMapping::containingSquashedLayer. layoutObject happens to be NULL in some cases. This CL fixes an undedefined behavior by avoiding making calls on a null object. BUG=662265 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
krasin@chromium.org changed reviewers: + thakis@chromium.org
mstensho@opera.com changed reviewers: + chrishtr@chromium.org
I don't know whether it's right to call this method with a nullptr, but chrishtr hopefully knows. There's a typo in your commit message: "undedefined" If we figure out what actually goes wrong here, maybe it's even possible to write a test that would fail in normal builds too.
Description was changed from ========== Naive fix of a call on a nil object in CompositedLayerMapping::containingSquashedLayer. layoutObject happens to be NULL in some cases. This CL fixes an undedefined behavior by avoiding making calls on a null object. BUG=662265 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Naive fix of a call on a nil object in CompositedLayerMapping::containingSquashedLayer. layoutObject happens to be NULL in some cases. This CL fixes an undefined behavior by avoiding making calls on a null object. BUG=662265 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
On 2016/11/04 09:30:12, mstensho wrote: > I don't know whether it's right to call this method with a nullptr, but chrishtr > hopefully knows. > > There's a typo in your commit message: "undedefined" Thank you, fixed. > > If we figure out what actually goes wrong here, maybe it's even possible to > write a test that would fail in normal builds too. I am all for that, but I need more input from the code owners, of what is actually wrong here. My investigations can be found here: https://bugs.chromium.org/p/chromium/issues/detail?id=662265
The CQ bit was checked by chrishtr@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hi Chris, thank you for the approval. Before we go forward and submit the CL, do you think the fix is the right one (on the callee, and not the caller side)? Shall I add a test, and if yes, where should it go? krasin
The CQ bit was unchecked by krasin@chromium.org
I think callee is fine in this case. You should be able to construct a layout test that exhibits the issue. You should be able to extract it from the failing browser test referenced in the bug.
The CQ bit was checked by krasin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/04 18:35:20, chrishtr wrote: > I think callee is fine in this case. You should be able to construct a layout > test that > exhibits the issue. You should be able to extract it from the failing browser > test referenced > in the bug. Okay, thank you. In this case, I am comfortable to submit this. Would you like me to add a test, or it's okay to rely on UBSan bot that sees this during the normal run?
It would be nice to have a test. Have you tried dumping the HTML of the browser test that fails with ubsan?
The CQ bit was unchecked by krasin@chromium.org
On 2016/11/04 18:48:20, chrishtr wrote: > It would be nice to have a test. Have you tried dumping the HTML of the browser > test that > fails with ubsan? How do I do that? Any tricks? Also, assuming I have dumped the HTML, where should I put it to have a test for this? (Sorry, I am mostly working from the compiler side, and not really a WebKit / Blink expert)
The CQ bit was checked by krasin@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.
Description was changed from ========== Naive fix of a call on a nil object in CompositedLayerMapping::containingSquashedLayer. layoutObject happens to be NULL in some cases. This CL fixes an undefined behavior by avoiding making calls on a null object. BUG=662265 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Naive fix of a call on a nil object in CompositedLayerMapping::containingSquashedLayer. layoutObject happens to be NULL in some cases. This CL fixes an undefined behavior by avoiding making calls on a null object. BUG=662265 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Naive fix of a call on a nil object in CompositedLayerMapping::containingSquashedLayer. layoutObject happens to be NULL in some cases. This CL fixes an undefined behavior by avoiding making calls on a null object. BUG=662265 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Naive fix of a call on a nil object in CompositedLayerMapping::containingSquashedLayer. layoutObject happens to be NULL in some cases. This CL fixes an undefined behavior by avoiding making calls on a null object. BUG=662265 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/a0c9881a5470c0af5ba0faefb785e6c0751c669e Cr-Commit-Position: refs/heads/master@{#430024} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a0c9881a5470c0af5ba0faefb785e6c0751c669e Cr-Commit-Position: refs/heads/master@{#430024} |
