99142 2012-10-11 23:33:52 -0700 fileReader abort case causes Chromium renderer crash 2012-10-26 02:38:15 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P1 Major --- 0 ningxin.hu webkit-unassigned ap ericu haraken japhet jianli kinuko li.yin webkit.review.bot oldest_to_newest 740653 0 ningxin.hu 2012-10-11 23:33:52 -0700 Reproduce steps: 1. Build Chromium 24.0.1292.0, which includes WebKit 537.14 (trunk@130891) 2. Launch Chromium to load the attached fileReader abort case file-reader-abort-test.html 3. After test runs, wait a couple of minutes, renderer will crash. 740655 1 ningxin.hu 2012-10-11 23:39:59 -0700 From the test logging: ///////////////////////////////// Received loadstart event Received load event Received loadend event DONE Received error event: 3 Received abort event Received loadend event DONE //////////////////////////////// load event is triggered before abort event. 740656 2 ningxin.hu 2012-10-11 23:41:26 -0700 After investigation of the code, it seems FileReader::didFinishLoading() doesn't take care of aborting case. I will try to upload a patch for this. 740657 3 ningxin.hu 2012-10-11 23:43:53 -0700 However, run fast/files/file-reader-abort.html in DumpRenderTree will not cause the crash. It might be related to Chromium's multiprocess loading architecture. 740658 4 168366 ningxin.hu 2012-10-11 23:44:43 -0700 Created attachment 168366 test case 740697 5 168373 ningxin.hu 2012-10-12 00:52:43 -0700 Created attachment 168373 Patch 740705 6 168373 haraken 2012-10-12 01:05:30 -0700 Comment on attachment 168373 Patch The change looks reasonable, but would you add a test case that repros the crash? As far as I read the comments on the bug, fast/files/file-reader-abort.html is not crashing in DumpRenderTree. 740712 7 ningxin.hu 2012-10-12 01:16:02 -0700 (In reply to comment #6) Yes. fast/files/file-reader-abort.html doesn't crash DumpRenderTree. But If run test case attached in Chromium browser, it will crash. The attached test case has essential same logic as fast/files/file-reader-abort.html. So I commented that there is no new test case need to be added. If we could let Chromium (with multiprocess) instead of DumpRenderTree to run the test, this crash should be covered. Do we have this way? > (From update of attachment 168373 [details]) > The change looks reasonable, but would you add a test case that repros the crash? As far as I read the comments on the bug, fast/files/file-reader-abort.html is not crashing in DumpRenderTree. 740727 8 168373 haraken 2012-10-12 01:45:30 -0700 Comment on attachment 168373 Patch Thanks for the clarification. > If we could let Chromium (with multiprocess) instead of DumpRenderTree to run the test, this crash should be covered. Do we have this way? I don't know... Given that the change looks reasonable and that you've manually tested, it would be OK to land it without tests. (It would be better to describe the situation in ChangeLog, instead of "No new tests".) 740738 9 168380 ningxin.hu 2012-10-12 02:12:31 -0700 Created attachment 168380 Patch 740739 10 168380 haraken 2012-10-12 02:12:59 -0700 Comment on attachment 168380 Patch Thanks 740740 11 ningxin.hu 2012-10-12 02:15:13 -0700 I found the fast/files/file-reader-abort.html could be load in Chromium browser to reproduce this issue. So I updated the ChangeLog. Please review. Thanks. 740972 12 ap 2012-10-12 09:29:30 -0700 > I don't know... Given that the change looks reasonable and that you've manually tested, it would be OK to land it without tests. (It would be better to describe the situation in ChangeLog, instead of "No new tests".) Could you please elaborate? I don't see how either would be an excuse for not making a regression test. 741698 13 haraken 2012-10-14 20:24:49 -0700 (In reply to comment #12) > > I don't know... Given that the change looks reasonable and that you've manually tested, it would be OK to land it without tests. (It would be better to describe the situation in ChangeLog, instead of "No new tests".) > > Could you please elaborate? I don't see how either would be an excuse for not making a regression test. Ningxin Hu: I confirmed that the failure doesn't happen on DRT, but would you explain why the failure happens in a multi-threaded environment only? Also, maybe you can write a regression test using workers? 741760 14 ningxin.hu 2012-10-14 23:02:15 -0700 (In reply to comment #13) > (In reply to comment #12) > > > I don't know... Given that the change looks reasonable and that you've manually tested, it would be OK to land it without tests. (It would be better to describe the situation in ChangeLog, instead of "No new tests".) > > > > Could you please elaborate? I don't see how either would be an excuse for not making a regression test. > > Ningxin Hu: I confirmed that the failure doesn't happen on DRT, but would you explain why the failure happens in a multi-threaded environment only? Also, maybe you can write a regression test using workers? Have you ever confirmed the failure in Chromium browser? It should be explicit since load event is fired before abort event. For the reason, I can figure out that after test calls FileReader.abort which invokes FileReader::abort. In FileReader::abort, it schedules delayedAbort to handle real aborting (including call FileReader::terminate). But when use Chromium to run the test case, FileReader::didFinishLoading is invoked before delayedAbort. So failure happens. I didn't mention it happens in multi-threaded environment. I mean it should be related to resource loading behavior difference. As you know, the DRT and Chromium uses different resource loader implementation, for example, DRT uses test_shell/simple_resource_loader_bridge.cc in single-process mode, however Chromium uses multi-process resource loading. So I think it doesn't help to have a worker to run this case. If it really needs a regression test, we need the test infrastructure to support, say to let Chromium run the layout tests. 741771 15 haraken 2012-10-14 23:36:23 -0700 (In reply to comment #14) > I didn't mention it happens in multi-threaded environment. I mean it should be related to resource loading behavior difference. As you know, the DRT and Chromium uses different resource loader implementation, for example, DRT uses test_shell/simple_resource_loader_bridge.cc in single-process mode, however Chromium uses multi-process resource loading. > So I think it doesn't help to have a worker to run this case. If it really needs a regression test, we need the test infrastructure to support, say to let Chromium run the layout tests. Thank you very much for the clarification! - ccing japhet, who knows well of resource loading. - I'd like to delegate cq+ for ap. 743767 16 ningxin.hu 2012-10-16 18:26:44 -0700 (In reply to comment #15) > > Thank you very much for the clarification! > > - ccing japhet, who knows well of resource loading. > > - I'd like to delegate cq+ for ap. Hi japhet, do you have any comments? Thanks in advance. 744999 17 kinuko 2012-10-17 23:04:24 -0700 > > So I think it doesn't help to have a worker to run this case. If it really needs a regression test, we need the test infrastructure to support, say to let Chromium run the layout tests. (Drive-by) at least we can run more File API / storage layout tests with chromium code (but not with DRT code) in chrome side. Filed an issue: http://code.google.com/p/chromium/issues/detail?id=156563 751417 18 li.yin 2012-10-25 18:39:32 -0700 It seems file-reader-abort.html has been added into content test of chromium already. I think we can add this patch into cq now, after all the crash looks so ugly. 751422 19 168380 webkit.review.bot 2012-10-25 18:41:48 -0700 Comment on attachment 168380 Patch Rejecting attachment 168380 from commit-queue. li.yin@intel.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights. 751430 20 li.yin 2012-10-25 18:53:06 -0700 Sorry, I still have no the cq+ permissions :( Let's wait some guys to help do that. 751601 21 haraken 2012-10-25 23:40:57 -0700 (In reply to comment #18) > It seems file-reader-abort.html has been added into content test of chromium already. Would you tell me where? > I think we can add this patch into cq now, after all the crash looks so ugly. I'll cq+ it after confirming the test in the chromium side. 751617 22 li.yin 2012-10-26 00:01:23 -0700 (In reply to comment #21) > (In reply to comment #18) > > It seems file-reader-abort.html has been added into content test of chromium already. > > Would you tell me where? > > > I think we can add this patch into cq now, after all the crash looks so ugly. > > I'll cq+ it after confirming the test in the chromium side. The test can be found from "chromium-trunk/src/content/test/data/layout_tests/LayoutTests/fast/files". They are added into chromium code through gclient sync. 751730 23 168380 webkit.review.bot 2012-10-26 02:38:10 -0700 Comment on attachment 168380 Patch Clearing flags on attachment: 168380 Committed r132592: <http://trac.webkit.org/changeset/132592> 751731 24 webkit.review.bot 2012-10-26 02:38:15 -0700 All reviewed patches have been landed. Closing bug. 168366 2012-10-11 23:44:43 -0700 2012-10-11 23:44:43 -0700 test case file-reader-abort-test.html text/html 844 ningxin.hu CjwhRE9DVFlQRSBodG1sPgo8aHRtbCBsYW5nPSJlbiI+CjxoZWFkPgo8L2hlYWQ+Cjxib2R5Pgo8 dGl0bGU+ZmlsZXJlYWRlciB0ZXN0IC0gRmlsZVJlYWRlciBhYm9ydDwvdGl0bGU+CjxkaXYgaWQ9 ImxvZyI+PC9kaXY+CjwvYm9keT4KPHNjcmlwdD4KZnVuY3Rpb24gd3JpdGVMb2cobXNnKSB7Cgl2 YXIgbm9kZSA9IGRvY3VtZW50LmdldEVsZW1lbnRCeUlkKCJsb2ciKTsKCW5vZGUuaW5uZXJIVE1M ICs9IG1zZyArICc8L2JyPic7Cn0KCnZhciBibG9iID0gbmV3IEJsb2IoWyJmaWxlUmVhZGVyIHJl YWRBc1RleHQgaXMgT0siXSwge3R5cGU6ICd0ZXh0L3BsYWluJ30pOwp2YXIgcmVhZGVyID0gbmV3 IEZpbGVSZWFkZXIoKTsKcmVhZGVyLnJlYWRBc1RleHQoYmxvYik7IAoKcmVhZGVyLm9ubG9hZHN0 YXJ0ID0gZnVuY3Rpb24oKSB7CiAgICB3cml0ZUxvZygiUmVjZWl2ZWQgbG9hZHN0YXJ0IGV2ZW50 Iik7CiAgICByZWFkZXIuYWJvcnQoKTsKfTsKCnJlYWRlci5vbmxvYWQgPSBmdW5jdGlvbigpIHsK ICAgIHdyaXRlTG9nKCJSZWNlaXZlZCBsb2FkIGV2ZW50Iik7Cn07CgpyZWFkZXIub25sb2FkZW5k ID0gZnVuY3Rpb24oKXsKICAgIHdyaXRlTG9nKCJSZWNlaXZlZCBsb2FkZW5kIGV2ZW50Iik7CiAg ICB3cml0ZUxvZygiRE9ORSIpOwp9OwoKcmVhZGVyLm9uYWJvcnQgPSBmdW5jdGlvbigpIHsKICAg IHdyaXRlTG9nKCJSZWNlaXZlZCBhYm9ydCBldmVudCIpOwp9OwoKcmVhZGVyLm9uZXJyb3IgPSBm dW5jdGlvbihldmVudCkgewogICAgd3JpdGVMb2coIlJlY2VpdmVkIGVycm9yIGV2ZW50OiAiICsg ZXZlbnQudGFyZ2V0LmVycm9yLmNvZGUpOwp9Owo8L3NjcmlwdD4KPC9odG1sPg== 168373 2012-10-12 00:52:43 -0700 2012-10-12 02:12:23 -0700 Patch bug-99142-20121012155509.patch text/plain 1304 ningxin.hu U3VidmVyc2lvbiBSZXZpc2lvbjogMTMwODkxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggM2RkY2YzZTY0N2UxY2M4 YjdhNDg3ZTBlZjFjNGQ0ZjNhOGI2YzUxYi4uZDRjMmU0OWRhMzczYWU0NjMxM2Y4MTE5NmMzMjRl YjU2YjdkYjE1YSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDEyLTEwLTEyICBOaW5n eGluIEh1ICA8bmluZ3hpbi5odUBpbnRlbC5jb20+CisKKyAgICAgICAgRmlsZVJlYWRlciBhYm9y dCBjYXNlIGNhdXNlcyBDaHJvbWl1bSByZW5kZXJlciBjcmFzaAorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTkxNDIKKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBCZWZvcmUgZGlzcGF0Y2hpbmcgbG9hZCBldmVu dCwgRmlsZVJlYWRlciBzaG91bGQgY2hlY2sgaWYgaXQgaXMgCisgICAgICAgIGluIGFib3J0aW5n LgorCisgICAgICAgIE5vIG5ldyB0ZXN0cy4gQ292ZXJlZCBieSBmYXN0L2ZpbGVzL2ZpbGUtcmVh ZGVyLWFib3J0Lmh0bWwKKworICAgICAgICAqIGZpbGVhcGkvRmlsZVJlYWRlci5jcHA6CisgICAg ICAgIChXZWJDb3JlOjpGaWxlUmVhZGVyOjpkaWRGaW5pc2hMb2FkaW5nKToKKwogMjAxMi0xMC0x MCAgQ2hyaXN0b3BoZSBEdW1leiAgPGNocmlzdG9waGUuZHVtZXpAaW50ZWwuY29tPgogCiAgICAg ICAgIFNRTFJlc3VsdFNldC5yb3dzQWZmZWN0ZWQgbm90IGNsZWFyZWQKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL2ZpbGVhcGkvRmlsZVJlYWRlci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9maWxl YXBpL0ZpbGVSZWFkZXIuY3BwCmluZGV4IDI0NzY4NTlmYmE1ZWU2MDQ2NTJmZjZlYWI4NDhkYTY5 MTVkOTQ1Y2EuLjEyOGViNjQ1YzZlN2Y1OGZiZjQwN2MzMjE5MTYzMTZmMzA4NjA3MzcgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2ZpbGVhcGkvRmlsZVJlYWRlci5jcHAKKysrIGIvU291cmNl L1dlYkNvcmUvZmlsZWFwaS9GaWxlUmVhZGVyLmNwcApAQCAtMjE1LDYgKzIxNSw5IEBAIHZvaWQg RmlsZVJlYWRlcjo6ZGlkUmVjZWl2ZURhdGEoKQogCiB2b2lkIEZpbGVSZWFkZXI6OmRpZEZpbmlz aExvYWRpbmcoKQogeworICAgIGlmIChtX2Fib3J0aW5nKQorICAgICAgICByZXR1cm47CisKICAg ICBBU1NFUlQobV9zdGF0ZSAhPSBET05FKTsKICAgICBtX3N0YXRlID0gRE9ORTsKIAo= 168380 2012-10-12 02:12:31 -0700 2012-10-26 02:38:10 -0700 Patch bug-99142-20121012171458.patch text/plain 1409 ningxin.hu U3VidmVyc2lvbiBSZXZpc2lvbjogMTMwODkxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggM2RkY2YzZTY0N2UxY2M4 YjdhNDg3ZTBlZjFjNGQ0ZjNhOGI2YzUxYi4uYjU2YmYzYjU2NWJmODBmNTQyNzgwN2JiZTVmOWU0 YTRjM2YyZDNiOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDEyLTEwLTEyICBOaW5n eGluIEh1ICA8bmluZ3hpbi5odUBpbnRlbC5jb20+CisKKyAgICAgICAgRmlsZVJlYWRlciBhYm9y dCBjYXNlIGNhdXNlcyBDaHJvbWl1bSByZW5kZXJlciBjcmFzaAorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTkxNDIKKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBCZWZvcmUgZGlzcGF0Y2hpbmcgbG9hZCBldmVu dCwgRmlsZVJlYWRlciBzaG91bGQgY2hlY2sgaWYgaXQgaXMKKyAgICAgICAgaW4gYWJvcnRpbmcu CisKKyAgICAgICAgVGVzdDogbWFudWFsbHkgbGF1bmNoIGZhc3QvZmlsZXMvZmlsZS1yZWFkZXIt YWJvcnQuaHRtbCBpbiBDaHJvbWl1bQorICAgICAgICBicm93c2VyIGFuZCBjb21wYXJlIHRoZSBy ZXN1bHQgd2l0aAorICAgICAgICBmYXN0L2ZpbGVzL2ZpbGUtcmVhZGVyLWFib3J0LWV4cGVjdGVk LnR4dC4KKworICAgICAgICAqIGZpbGVhcGkvRmlsZVJlYWRlci5jcHA6CisgICAgICAgIChXZWJD b3JlOjpGaWxlUmVhZGVyOjpkaWRGaW5pc2hMb2FkaW5nKToKKwogMjAxMi0xMC0xMCAgQ2hyaXN0 b3BoZSBEdW1leiAgPGNocmlzdG9waGUuZHVtZXpAaW50ZWwuY29tPgogCiAgICAgICAgIFNRTFJl c3VsdFNldC5yb3dzQWZmZWN0ZWQgbm90IGNsZWFyZWQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJD b3JlL2ZpbGVhcGkvRmlsZVJlYWRlci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9maWxlYXBpL0ZpbGVS ZWFkZXIuY3BwCmluZGV4IDI0NzY4NTlmYmE1ZWU2MDQ2NTJmZjZlYWI4NDhkYTY5MTVkOTQ1Y2Eu LjEyOGViNjQ1YzZlN2Y1OGZiZjQwN2MzMjE5MTYzMTZmMzA4NjA3MzcgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJDb3JlL2ZpbGVhcGkvRmlsZVJlYWRlci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUv ZmlsZWFwaS9GaWxlUmVhZGVyLmNwcApAQCAtMjE1LDYgKzIxNSw5IEBAIHZvaWQgRmlsZVJlYWRl cjo6ZGlkUmVjZWl2ZURhdGEoKQogCiB2b2lkIEZpbGVSZWFkZXI6OmRpZEZpbmlzaExvYWRpbmco KQogeworICAgIGlmIChtX2Fib3J0aW5nKQorICAgICAgICByZXR1cm47CisKICAgICBBU1NFUlQo bV9zdGF0ZSAhPSBET05FKTsKICAgICBtX3N0YXRlID0gRE9ORTsKIAo=