Bug 96354 - WebFrameImpl::client() needs NULL check in WebWorkerClientImpl::openFileSystem
Summary: WebFrameImpl::client() needs NULL check in WebWorkerClientImpl::openFileSystem
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Taiju Tsuiki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-10 22:48 PDT by Taiju Tsuiki
Modified: 2012-09-12 08:30 PDT (History)
5 users (show)

See Also:

Attachments
Patch (1.74 KB, patch)
2012-09-10 22:50 PDT, Taiju Tsuiki 		no flags Details | Formatted Diff | Diff
build fix (1.96 KB, patch)
2012-09-10 23:15 PDT, Taiju Tsuiki 		no flags Details | Formatted Diff | Diff
add test (5.33 KB, patch)
2012-09-11 03:35 PDT, Taiju Tsuiki 		no flags Details | Formatted Diff | Diff
fix style (5.38 KB, patch)
2012-09-11 05:24 PDT, Taiju Tsuiki 		no flags Details | Formatted Diff | Diff
simplify test (5.36 KB, patch)
2012-09-11 21:27 PDT, Taiju Tsuiki 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.