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

Taiju Tsuiki
Reported 2012-09-10 22:48:59 PDT
We need to add it to avoid NULL pointer dereference. http://crbug.com/147592
Attachments
Patch (1.74 KB, patch)
2012-09-10 22:50 PDT, Taiju Tsuiki
no flags
build fix (1.96 KB, patch)
2012-09-10 23:15 PDT, Taiju Tsuiki
no flags
add test (5.33 KB, patch)
2012-09-11 03:35 PDT, Taiju Tsuiki
no flags
fix style (5.38 KB, patch)
2012-09-11 05:24 PDT, Taiju Tsuiki
no flags
simplify test (5.36 KB, patch)
2012-09-11 21:27 PDT, Taiju Tsuiki
no flags
Taiju Tsuiki
Comment 1 2012-09-10 22:50:20 PDT
Taiju Tsuiki
Comment 2 2012-09-10 23:15:10 PDT
Created attachment 163285 [details] build fix
Kent Tamura
Comment 3 2012-09-10 23:35:14 PDT
Comment on attachment 163285 [details] build fix Why does the patch have no tests?
Taiju Tsuiki
Comment 4 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.
Kent Tamura
Comment 5 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.
Taiju Tsuiki
Comment 6 2012-09-11 03:35:49 PDT
Created attachment 163323 [details] add test
Taiju Tsuiki
Comment 7 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?
Kent Tamura
Comment 8 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.
Taiju Tsuiki
Comment 9 2012-09-11 05:24:14 PDT
Created attachment 163339 [details] fix style
Taiju Tsuiki
Comment 10 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
Kent Tamura
Comment 11 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?
Taiju Tsuiki
Comment 12 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.
Taiju Tsuiki
Comment 13 2012-09-11 21:27:33 PDT
Created attachment 163513 [details] simplify test
Kent Tamura
Comment 14 2012-09-11 21:43:05 PDT
Comment on attachment 163513 [details] simplify test rubber stamped
WebKit Review Bot
Comment 15 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>
WebKit Review Bot
Comment 16 2012-09-11 22:02:30 PDT
All reviewed patches have been landed. Closing bug.
Adam Klein
Comment 17 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.
Adam Klein
Comment 18 2012-09-12 08:30:33 PDT
Opened bug 96524 to track flakiness.
Note You need to log in before you can comment on or make changes to this bug.