Bug 96354

Summary: WebFrameImpl::client() needs NULL check in WebWorkerClientImpl::openFileSystem
Product: WebKit Reporter: Taiju Tsuiki <tzik>
Component: WebKit Misc.Assignee: Taiju Tsuiki <tzik>
Status: RESOLVED FIXED    
Severity: Normal CC: adamk, jamesr, kinuko, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
build fix
none
add test
none
fix style
none
simplify test none

Description Taiju Tsuiki 2012-09-10 22:48:59 PDT 
We need to add it to avoid NULL pointer dereference.
http://crbug.com/147592
Comment 1 Taiju Tsuiki 2012-09-10 22:50:20 PDT 
Created attachment 163284 [details]
Patch
Comment 2 Taiju Tsuiki 2012-09-10 23:15:10 PDT 
Created attachment 163285 [details]
build fix
Comment 3 Kent Tamura 2012-09-10 23:35:14 PDT 
Comment on attachment 163285 [details]
build fix

Why does the patch have no tests?
Comment 4 Taiju Tsuiki 2012-09-11 01:03:20 PDT 
We know we hit renderer crash here and I believe this patch will fix it.
But, I could not reproduce it locally nor reduce it to simple test case.
Comment 5 Kent Tamura 2012-09-11 01:08:00 PDT 
(In reply to comment #4)
> But, I could not reproduce it locally nor reduce it to simple test case.

ChangeLog should have such information.
Comment 6 Taiju Tsuiki 2012-09-11 03:35:49 PDT 
Created attachment 163323 [details]
add test
Comment 7 Taiju Tsuiki 2012-09-11 03:38:00 PDT 
(In reply to comment #5)
> (In reply to comment #4)
> > But, I could not reproduce it locally nor reduce it to simple test case.
> 
> ChangeLog should have such information.

I see. I'll do it next time.
Now, I added a regression test for it. WebKit crashes on the test without my fix.
Could you take a look again?
Comment 8 Kent Tamura 2012-09-11 03:43:03 PDT 
Comment on attachment 163323 [details]
add test

View in context: https://bugs.webkit.org/attachment.cgi?id=163323&action=review

> LayoutTests/fast/filesystem/workers/detached-frame-crash.html:6
> +if (window.testRunner) {
> +  testRunner.dumpAsText();

We use 4-space indentation in JavaScript code too.
Comment 9 Taiju Tsuiki 2012-09-11 05:24:14 PDT 
Created attachment 163339 [details]
fix style
Comment 10 Taiju Tsuiki 2012-09-11 05:31:38 PDT 
Comment on attachment 163323 [details]
add test

View in context: https://bugs.webkit.org/attachment.cgi?id=163323&action=review

>> LayoutTests/fast/filesystem/workers/detached-frame-crash.html:6
>> +  testRunner.dumpAsText();
> 
> We use 4-space indentation in JavaScript code too.

Done
Comment 11 Kent Tamura 2012-09-11 19:27:53 PDT 
Comment on attachment 163339 [details]
fix style

View in context: https://bugs.webkit.org/attachment.cgi?id=163339&action=review

> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:214
> +        callbacks->didFail(WebFileErrorAbort);

Can you confirm this failure in the test?
Comment 12 Taiju Tsuiki 2012-09-11 21:12:34 PDT 
(In reply to comment #11)
> (From update of attachment 163339 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163339&action=review
> 
> > Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:214
> > +        callbacks->didFail(WebFileErrorAbort);
> 
> Can you confirm this failure in the test?

I couldn't. I think there is no way to propagate error message outside from a worker on detached frame.
Comment 13 Taiju Tsuiki 2012-09-11 21:27:33 PDT 
Created attachment 163513 [details]
simplify test
Comment 14 Kent Tamura 2012-09-11 21:43:05 PDT 
Comment on attachment 163513 [details]
simplify test

rubber stamped
Comment 15 WebKit Review Bot 2012-09-11 22:02:26 PDT 
Comment on attachment 163513 [details]
simplify test

Clearing flags on attachment: 163513

Committed r128263: <http://trac.webkit.org/changeset/128263>
Comment 16 WebKit Review Bot 2012-09-11 22:02:30 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 17 Adam Klein 2012-09-12 08:26:15 PDT 
One of the tests added here, fast/filesystem/workers/detached-frame-crash.html, has been flaky in debug builds on Win/Linux since it was added. See flakiness dashboard:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Ffilesystem%2Fworkers%2Fdetached-frame-crash.html

Please take a look.
Comment 18 Adam Klein 2012-09-12 08:30:33 PDT 
Opened bug 96524 to track flakiness.