Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2844593002:
Protect against lifecycle updates that delete a layout object for autoscroll (Closed)
DescriptionProtect against lifecycle updates that delete a layout object for autoscroll.
NOPRESUBMIT=true
BUG=713190
Review-Url: https://codereview.chromium.org/2844593002
Cr-Commit-Position: refs/heads/master@{#468109}
Committed: https://chromium.googlesource.com/chromium/src/+/e5b83af160ffb15e52163318bdd3aadb14c185c8
Patch Set 1 #Patch Set 2 : none #
Total comments: 3
Patch Set 3 : none #Patch Set 4 : none #Patch Set 5 : none #
Total comments: 8
Patch Set 6 : none #Patch Set 7 : none #
Total comments: 2
Patch Set 8 : none #
Total comments: 1
Messages
Total messages: 52 (33 generated)
Description was changed from ========== none none BUG= ========== to ========== Protect against lifecycle updates that delete a layout object for autoscroll. BUG=713190 ==========
The CQ bit was checked by chrishtr@chromium.org 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...
chrishtr@chromium.org changed reviewers: + mstensho@opera.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think we need a test case. https://codereview.chromium.org/2844593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLSelectElement.cpp (right): https://codereview.chromium.org/2844593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLSelectElement.cpp:1521: if (LayoutObject* object = GetLayoutObject()) I don't know if it matters, or if it's possible at all, but what if we don't have a layout object here? Should we still call UpdateAllLifecyclePhasesExceptPaint(), perhaps?
On 2017/04/26 at 09:46:28, mstensho wrote: > I think we need a test case. Done. > > https://codereview.chromium.org/2844593002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLSelectElement.cpp (right): > > https://codereview.chromium.org/2844593002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLSelectElement.cpp:1521: if (LayoutObject* object = GetLayoutObject()) > I don't know if it matters, or if it's possible at all, but what if we don't have a layout object here? Should we still call UpdateAllLifecyclePhasesExceptPaint(), perhaps?
https://codereview.chromium.org/2844593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLSelectElement.cpp (right): https://codereview.chromium.org/2844593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLSelectElement.cpp:1521: if (LayoutObject* object = GetLayoutObject()) On 2017/04/26 at 09:46:28, mstensho wrote: > I don't know if it matters, or if it's possible at all, but what if we don't have a layout object here? Should we still call UpdateAllLifecyclePhasesExceptPaint(), perhaps? StartAutoscrollForSelection will be a no-op if layout_object is null, so it's ok to not run the lifecycle.
The CQ bit was checked by chrishtr@chromium.org 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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by chrishtr@chromium.org 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: 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 chrishtr@chromium.org 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.
https://codereview.chromium.org/2844593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLSelectElement.cpp (right): https://codereview.chromium.org/2844593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLSelectElement.cpp:1521: if (LayoutObject* object = GetLayoutObject()) On 2017/04/26 23:11:06, chrishtr wrote: > On 2017/04/26 at 09:46:28, mstensho wrote: > > I don't know if it matters, or if it's possible at all, but what if we don't > have a layout object here? Should we still call > UpdateAllLifecyclePhasesExceptPaint(), perhaps? > > StartAutoscrollForSelection will be a no-op if layout_object is null, so it's ok > to not run > the lifecycle. I was more thinking of the possibility of a layout object appearing for the select element if you run the life cycle. But yeah, that does sound weird and useless, so fine. :) https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html (right): https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:2: Test passes if it does not crash.<br> I can't get it to crash (without the patch). Also, it should be converted to a testharness test, so that you don't need the -expected.txt counterpart. https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:34: }, 1000); 1000ms is quite a lot. Is it really necessary? https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/MouseEventManager.cpp:754: if (LayoutObject* next_layout_object = target_node->GetLayoutObject()) { I suppose you could also re-use layout_object, instead of keeping a pointer to a potentially dead object around?
chrishtr@chromium.org changed reviewers: + lanwei@chromium.org
Everything now done, except that the test doesn't work with chrome.gpuBenchmarking.pointerActionSequence, which I am now forced to use after https://chromium.googlesource.com/chromium/src/+/a15a2d8062d9919bef96f18455a0... lanwei, could you please tell me what I am doing wrong? I can get the mouse events to pass through with eventSender, but not the new API. See patchset 3. https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html (right): https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:2: Test passes if it does not crash.<br> On 2017/04/27 at 20:16:11, mstensho wrote: > I can't get it to crash (without the patch). Fixed - the uploaded version has a one-character typo (missing > after style) > > Also, it should be converted to a testharness test, so that you don't need the -expected.txt counterpart. How exactly do I do that? including testharness doesn't omit the expected file, is there a method I should run? https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:34: }, 1000); On 2017/04/27 at 20:16:11, mstensho wrote: > 1000ms is quite a lot. Is it really necessary? Found a way to reduce to 50ms. https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/MouseEventManager.cpp:754: if (LayoutObject* next_layout_object = target_node->GetLayoutObject()) { On 2017/04/27 at 20:16:11, mstensho wrote: > I suppose you could also re-use layout_object, instead of keeping a pointer to a potentially dead object around? Done.
The CQ bit was checked by chrishtr@chromium.org 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/04/28 02:21:35, chrishtr wrote: > Everything now done, except that the test doesn't work with > chrome.gpuBenchmarking.pointerActionSequence, which I am now forced > to use after > https://chromium.googlesource.com/chromium/src/+/a15a2d8062d9919bef96f18455a0... > > lanwei, could you please tell me what I am doing wrong? I can get the mouse > events to pass through with eventSender, but not the new API. See patchset 3. > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html > (right): > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:2: Test > passes if it does not crash.<br> > On 2017/04/27 at 20:16:11, mstensho wrote: > > I can't get it to crash (without the patch). > > Fixed - the uploaded version has a one-character typo (missing > after style) > > > > > Also, it should be converted to a testharness test, so that you don't need the > -expected.txt counterpart. > > How exactly do I do that? including testharness doesn't omit the expected file, > is there a method > I should run? > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:34: }, > 1000); > On 2017/04/27 at 20:16:11, mstensho wrote: > > 1000ms is quite a lot. Is it really necessary? > > Found a way to reduce to 50ms. > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/MouseEventManager.cpp:754: if > (LayoutObject* next_layout_object = target_node->GetLayoutObject()) { > On 2017/04/27 at 20:16:11, mstensho wrote: > > I suppose you could also re-use layout_object, instead of keeping a pointer to > a potentially dead object around? > > Done. should you be chaining your test together with the callbacks? With eventSender the calls were synchronous. With pointerActionSeqeunce you have a callback that is called when the event have been sent. The reload may fire before the events are actually delivered in your test. But if the callback was actually the reload then it wouldn't.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Yeah, chrome.gpuBenchmarking.pointerActionSequence() sure is a strange way of firing mouse events (eventSender sort of was a more obvious name), but I understand now why you have to. I really wondered what the GPU had to do with the price of eggs. https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html (right): https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:2: Test passes if it does not crash.<br> On 2017/04/28 02:21:34, chrishtr wrote: > On 2017/04/27 at 20:16:11, mstensho wrote: > > Also, it should be converted to a testharness test, so that you don't need the > -expected.txt counterpart. > > How exactly do I do that? including testharness doesn't omit the expected file, > is there a method > I should run? Take a look at e.g. LayoutTests/fast/events/focus-iframe-crash.html https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:34: }, 1000); On 2017/04/28 02:21:34, chrishtr wrote: > On 2017/04/27 at 20:16:11, mstensho wrote: > > 1000ms is quite a lot. Is it really necessary? > > Found a way to reduce to 50ms. W00t!! :)
On 2017/04/28 at 07:46:48, mstensho wrote: > Yeah, chrome.gpuBenchmarking.pointerActionSequence() sure is a strange way of firing mouse events (eventSender sort of was a more obvious name), but I understand now why you have to. I really wondered what the GPU had to do with the price of eggs. > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html (right): > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:2: Test passes if it does not crash.<br> > On 2017/04/28 02:21:34, chrishtr wrote: > > On 2017/04/27 at 20:16:11, mstensho wrote: > > > Also, it should be converted to a testharness test, so that you don't need the > > -expected.txt counterpart. > > > > How exactly do I do that? including testharness doesn't omit the expected file, > > is there a method > > I should run? > > Take a look at e.g. LayoutTests/fast/events/focus-iframe-crash.html Thanks - done. > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:34: }, 1000); > On 2017/04/28 02:21:34, chrishtr wrote: > > On 2017/04/27 at 20:16:11, mstensho wrote: > > > 1000ms is quite a lot. Is it really necessary? > > > > Found a way to reduce to 50ms. > > W00t!! :)
On 2017/04/28 at 02:34:45, dtapuska wrote: > On 2017/04/28 02:21:35, chrishtr wrote: > > Everything now done, except that the test doesn't work with > > chrome.gpuBenchmarking.pointerActionSequence, which I am now forced > > to use after > > https://chromium.googlesource.com/chromium/src/+/a15a2d8062d9919bef96f18455a0... > > > > lanwei, could you please tell me what I am doing wrong? I can get the mouse > > events to pass through with eventSender, but not the new API. See patchset 3. > > > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... > > File third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html > > (right): > > > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:2: Test > > passes if it does not crash.<br> > > On 2017/04/27 at 20:16:11, mstensho wrote: > > > I can't get it to crash (without the patch). > > > > Fixed - the uploaded version has a one-character typo (missing > after style) > > > > > > > > Also, it should be converted to a testharness test, so that you don't need the > > -expected.txt counterpart. > > > > How exactly do I do that? including testharness doesn't omit the expected file, > > is there a method > > I should run? > > > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:34: }, > > 1000); > > On 2017/04/27 at 20:16:11, mstensho wrote: > > > 1000ms is quite a lot. Is it really necessary? > > > > Found a way to reduce to 50ms. > > > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): > > > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/input/MouseEventManager.cpp:754: if > > (LayoutObject* next_layout_object = target_node->GetLayoutObject()) { > > On 2017/04/27 at 20:16:11, mstensho wrote: > > > I suppose you could also re-use layout_object, instead of keeping a pointer to > > a potentially dead object around? > > > > Done. > > should you be chaining your test together with the callbacks? With eventSender the calls were synchronous. With pointerActionSeqeunce you have a callback that is called when the event have been sent. The reload may fire before the events are actually delivered in your test. But if the callback was actually the reload then it wouldn't. Changed to chain with the callbacks, as you suggested. The test still doesn't work. If I put eventSender calls in the various callbacks, it crashes. Any idea what I'm doing wrong at this point?
Description was changed from ========== Protect against lifecycle updates that delete a layout object for autoscroll. BUG=713190 ========== to ========== Protect against lifecycle updates that delete a layout object for autoscroll. BUG=713190 ==========
dtapuska@chromium.org changed reviewers: + nzolghadr@chromium.org
On 2017/04/28 15:48:58, chrishtr wrote: > On 2017/04/28 at 02:34:45, dtapuska wrote: > > On 2017/04/28 02:21:35, chrishtr wrote: > > > Everything now done, except that the test doesn't work with > > > chrome.gpuBenchmarking.pointerActionSequence, which I am now forced > > > to use after > > > > https://chromium.googlesource.com/chromium/src/+/a15a2d8062d9919bef96f18455a0... > > > > > > lanwei, could you please tell me what I am doing wrong? I can get the mouse > > > events to pass through with eventSender, but not the new API. See patchset > 3. > > > > > > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... > > > File third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html > > > (right): > > > > > > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:2: > Test > > > passes if it does not crash.<br> > > > On 2017/04/27 at 20:16:11, mstensho wrote: > > > > I can't get it to crash (without the patch). > > > > > > Fixed - the uploaded version has a one-character typo (missing > after > style) > > > > > > > > > > > Also, it should be converted to a testharness test, so that you don't need > the > > > -expected.txt counterpart. > > > > > > How exactly do I do that? including testharness doesn't omit the expected > file, > > > is there a method > > > I should run? > > > > > > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:34: > }, > > > 1000); > > > On 2017/04/27 at 20:16:11, mstensho wrote: > > > > 1000ms is quite a lot. Is it really necessary? > > > > > > Found a way to reduce to 50ms. > > > > > > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): > > > > > > > https://codereview.chromium.org/2844593002/diff/80001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/input/MouseEventManager.cpp:754: if > > > (LayoutObject* next_layout_object = target_node->GetLayoutObject()) { > > > On 2017/04/27 at 20:16:11, mstensho wrote: > > > > I suppose you could also re-use layout_object, instead of keeping a > pointer to > > > a potentially dead object around? > > > > > > Done. > > > > should you be chaining your test together with the callbacks? With eventSender > the calls were synchronous. With pointerActionSeqeunce you have a callback that > is called when the event have been sent. The reload may fire before the events > are actually delivered in your test. But if the callback was actually the reload > then it wouldn't. > > Changed to chain with the callbacks, as you suggested. The test still doesn't > work. If I put eventSender calls in the various callbacks, it crashes. > Any idea what I'm doing wrong at this point? Added Navid... he might be able to provide some guidance. Lan and Navid are more familiar with this than I.
https://codereview.chromium.org/2844593002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html (right): https://codereview.chromium.org/2844593002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:30: setTimeout(moveGesture, 50); Unfortunately, pointerActionSequence API expects the actions are in a sequence. But you are doing a page load when the mouse is pressed, so when you call the moveGesture(), the mouse is not pressed anymore, it does not carry the previous mouse status. Sorry, it does not work at this moment, we will work on this later. You can use NOPRESUBMIT=true to ignore the presubmit error.
Description was changed from ========== Protect against lifecycle updates that delete a layout object for autoscroll. BUG=713190 ========== to ========== Protect against lifecycle updates that delete a layout object for autoscroll. NOPRESUBMIT=true BUG=713190 ==========
Updated. Now skips presubmit to allow eventSender. All other comments have been addressed, PTAL.
The CQ bit was checked by chrishtr@chromium.org 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...
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/2844593002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html (right): https://codereview.chromium.org/2844593002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:30: setTimeout(moveGesture, 50); On 2017/04/28 16:17:29, lanwei wrote: > Unfortunately, pointerActionSequence API expects the actions are in a sequence. > But you are doing a page load when the mouse is pressed, > so when you call the moveGesture(), the mouse is not pressed anymore, it does > not carry the previous mouse status. Sorry, it does not work at this moment, we > will work on this later. You can use NOPRESUBMIT=true to ignore the presubmit > error. It might be possible for this case to actually use a single pointerActionSequence (but actually do the reload inside a mousemove event listener). And then tell the pointer event sequence to pause for the 50ms.
On 2017/04/28 at 18:17:02, dtapuska wrote: > https://codereview.chromium.org/2844593002/diff/120001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html (right): > > https://codereview.chromium.org/2844593002/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:30: setTimeout(moveGesture, 50); > On 2017/04/28 16:17:29, lanwei wrote: > > Unfortunately, pointerActionSequence API expects the actions are in a sequence. > > But you are doing a page load when the mouse is pressed, > > so when you call the moveGesture(), the mouse is not pressed anymore, it does > > not carry the previous mouse status. Sorry, it does not work at this moment, we > > will work on this later. You can use NOPRESUBMIT=true to ignore the presubmit > > error. > > It might be possible for this case to actually use a single pointerActionSequence (but actually do the reload inside a mousemove event listener). And then tell the pointer event sequence to pause for the 50ms. Maybe, but I've already invested a ton of time in this CL...
On 2017/04/28 18:42:02, chrishtr wrote: > On 2017/04/28 at 18:17:02, dtapuska wrote: > > > https://codereview.chromium.org/2844593002/diff/120001/third_party/WebKit/Lay... > > File third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html > (right): > > > > > https://codereview.chromium.org/2844593002/diff/120001/third_party/WebKit/Lay... > > third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:30: > setTimeout(moveGesture, 50); > > On 2017/04/28 16:17:29, lanwei wrote: > > > Unfortunately, pointerActionSequence API expects the actions are in a > sequence. > > > But you are doing a page load when the mouse is pressed, > > > so when you call the moveGesture(), the mouse is not pressed anymore, it > does > > > not carry the previous mouse status. Sorry, it does not work at this moment, > we > > > will work on this later. You can use NOPRESUBMIT=true to ignore the > presubmit > > > error. > > > > It might be possible for this case to actually use a single > pointerActionSequence (but actually do the reload inside a mousemove event > listener). And then tell the pointer event sequence to pause for the 50ms. > > Maybe, but I've already invested a ton of time in this CL... don't worry about that. It is just for the future :-) lgtm
lgtm https://codereview.chromium.org/2844593002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html (right): https://codereview.chromium.org/2844593002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/autoscroll-select-crash.html:21: setTimeout(moveGesture, 50); Looks like this timeout is slightly too low for my machine. I get it to crash reliably if I use 55 instead. Never with 50. Sometimes with 53. This was when the Moon was in a waxing crescent phase. :-S
The CQ bit was unchecked by chrishtr@chromium.org
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493410701829840,
"parent_rev": "39206f00c8d9dd41a217c315fc0c0376f5848408", "commit_rev":
"e5b83af160ffb15e52163318bdd3aadb14c185c8"}
Message was sent while issue was closed.
Description was changed from ========== Protect against lifecycle updates that delete a layout object for autoscroll. NOPRESUBMIT=true BUG=713190 ========== to ========== Protect against lifecycle updates that delete a layout object for autoscroll. NOPRESUBMIT=true BUG=713190 Review-Url: https://codereview.chromium.org/2844593002 Cr-Commit-Position: refs/heads/master@{#468109} Committed: https://chromium.googlesource.com/chromium/src/+/e5b83af160ffb15e52163318bdd3... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/e5b83af160ffb15e52163318bdd3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
