Bug 96354 – WebFrameImpl::client() needs NULL check in WebWorkerClientImpl::openFileSystem

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

[Taiju Tsuiki]

We need to add it to avoid NULL pointer dereference.
http://crbug.com/147592

[Taiju Tsuiki]

Created attachment 163284 [details]
Patch

[Taiju Tsuiki]

Created attachment 163285 [details]
build fix

[Kent Tamura]

Comment on attachment 163285 [details]
build fix

Why does the patch have no tests?

[Taiju Tsuiki]

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]

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

Created attachment 163323 [details]
add test

[Taiju Tsuiki]

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

Created attachment 163339 [details]
fix style

[Taiju Tsuiki]

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

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

Created attachment 163513 [details]
simplify test

[Kent Tamura]

Comment on attachment 163513 [details]
simplify test

rubber stamped

[WebKit Review Bot]

Comment on attachment 163513 [details]
simplify test

Clearing flags on attachment: 163513

Committed r128263: <http://trac.webkit.org/changeset/128263>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Klein]

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]

Opened bug 96524 to track flakiness.