Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
288933002:
Fix crash after detaching document. (Closed)
|
Created:
6 years, 7 months ago by deepak.sa Modified:
6 years, 7 months ago CC:
blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionFix crash after detaching document.
If document is detached and click event is generated on
a label element, it causes a crash as the document doesn't
have frame. So adding additional check for LocalFrame.
BUG=370821
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174818
Patch Set 1 #Patch Set 2 : Added Layout test #
Total comments: 2
Patch Set 3 : Layout test updated #Patch Set 4 : Rebasing Issue #
Messages
Total messages: 30 (0 generated)
Please have a look. Thanks!
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deepak.sa@samsung.com/288933002/1
not lgtm. this needs a test.
On 2014/05/15 17:21:31, ojan wrote: > not lgtm. this needs a test. Err, that's my bad. I missed that the ClusterFuzz test wasn't attached.
The CQ bit was unchecked by esprehn@chromium.org
I have added the layout test. Please have another look. Thanks!
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: ojan@chromium.org. Please make sure to get positive review before another CQ attempt.
lgtm Please address the test comments before committing. Also, I wonder if a better fix would be to never call defaultEventHandler in the first place if there's no frame. That might not be correct though. Maybe there are cases where we need to fire events on documents with no frame. It's not obvious to me, so without digging further into it, this fix is fine. https://codereview.chromium.org/288933002/diff/20001/LayoutTests/fast/forms/l... File LayoutTests/fast/forms/label/detached-document.html (right): https://codereview.chromium.org/288933002/diff/20001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/detached-document.html:1: <html> Is the html element required for this test? If not, we usually leave it out. Also, does putting the <!DOCTYPE html> not work for some reason? https://codereview.chromium.org/288933002/diff/20001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/detached-document.html:6: try{var evtCF = document.createEvent('MouseEvent'); evtCF.initMouseEvent("click", 56644); tCF12.dispatchEvent(evtCF)}catch(e){} Please minimize this. It makes it easier for people who have to understand this test in the future. A few things can be done: 1. Remove duplicate variables. 2. Remove the try/cathhes 3. Move each statement to it's own line. 4. Give the variables less obscure names.
The CQ bit was checked by deepak.sa@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deepak.sa@samsung.com/288933002/40001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6050) linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/9451) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9250)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9254)
Please have a look again, as there were some rebasing issue. Thanks!
The CQ bit was checked by tkent@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deepak.sa@samsung.com/288933002/60001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/8904)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/8915)
The CQ bit was checked by deepak.sa@samsung.com
The CQ bit was unchecked by deepak.sa@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deepak.sa@samsung.com/288933002/60001
The CQ bit was checked by deepak.sa@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deepak.sa@samsung.com/288933002/60001
Message was sent while issue was closed.
Change committed as 174818 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
