Bug 99142 – fileReader abort case causes Chromium renderer crash

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ningxin Hu]

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.

[Ningxin Hu]

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.

[Ningxin Hu]

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.

[Ningxin Hu]

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.

[Ningxin Hu]

Created attachment 168366 [details]
test case

[Ningxin Hu]

Created attachment 168373 [details]
Patch

[Kentaro Hara]

Comment on attachment 168373 [details]
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.

[Ningxin Hu]

(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.

[Kentaro Hara]

Comment on attachment 168373 [details]
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".)

[Ningxin Hu]

Created attachment 168380 [details]
Patch

[Kentaro Hara]

Comment on attachment 168380 [details]
Patch

Thanks

[Ningxin Hu]

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.

[Alexey Proskuryakov]

> 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.

[Kentaro Hara]

(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?

[Ningxin Hu]

(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.

[Kentaro Hara]

(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.

[Ningxin Hu]

(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.

[Kinuko Yasuda]

> > 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

[Li Yin]

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.

[WebKit Review Bot]

Comment on attachment 168380 [details]
Patch

Rejecting attachment 168380 [details] from commit-queue.

[e-mail] 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.

[Li Yin]

Sorry, I still have no the cq+ permissions :(
Let's wait some guys to help do that.

[Kentaro Hara]

(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.

[Li Yin]

(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.

[WebKit Review Bot]

Comment on attachment 168380 [details]
Patch

Clearing flags on attachment: 168380

Committed r132592: <http://trac.webkit.org/changeset/132592>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.