Bug 76461 – DOMFileSystem::scheduleCallback() crashes with null scriptExecutionContext().

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

[Taiju Tsuiki]

Referring http://crbug.com/94895, WebKit crashes at the point.
I tried to fix the bug with the following patch.

[Taiju Tsuiki]

Created attachment 122776 [details]
Patch

[Darin Adler]

Comment on attachment 122776 [details]
Patch

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

> Source/WebCore/fileapi/DOMFileSystem.cpp:117
> +    if (!scriptExecutionContext())
> +        return;

I think we need to call the error callback in this case.

> Source/WebCore/fileapi/DOMFileSystem.h:101
> +    if (!scriptExecutionContext)
> +        return;

I think we need to call the error callback in this case.

[Taiju Tsuiki]

Created attachment 125090 [details]
Patch

[Taiju Tsuiki]

Error callback is unavailable in this point, and even if it is available, calling it may cause immediate invocation of callback.

The original bug report hits NULL scriptExecutionContext in GetPathCallback::didReadMetadata in DOMFileSystem.cpp.
I wander if we really need to use scheduleCallback here, that requires scriptExecutionContext.

All other use case of scheduleCallback looks for avoiding immediate invocation of the callback at start of async operation, where we can safely use scriptExecutionContext.
For our case, GetPathCallback::didReadMetatdata is invoked already delayed, so IMO we can invoke it directly like the patch.

[Taiju Tsuiki]

Could you take another look?

[Kinuko Yasuda]

This patch looks good to me.

[Kinuko Yasuda]

Comment on attachment 125090 [details]
Patch

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

This patch in general looks good to me.

> Source/WebCore/ChangeLog:11
> +

Could we add a test for this fix?  Otherwise please add a comment why it doesn't have a test.

> Source/WebCore/fileapi/DOMFileSystem.cpp:135
> +        if (m_successCallback) {

Early exit might be more commonly seen pattern?

if (!m_successCallback)
  return;

[David Levin]

Comment on attachment 125090 [details]
Patch

What Kinuko said.

[Taiju Tsuiki]

Created attachment 137840 [details]
Patch

[Taiju Tsuiki]

Created attachment 137842 [details]
Patch

[Taiju Tsuiki]

I fixed the style and added a test.
Could you take another look?

[Kinuko Yasuda]

Comment on attachment 137842 [details]
Patch

LGTM except for one concern.

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

> LayoutTests/fast/filesystem/file-after-reload-crash.html:12
> +var try_count = parseInt(sessionStorage['try']) || 0;

Might be better using simpler way like URL parameter rather than relying on other storage? (Also I'm not sure when sessionStorage expires in DRT)

[Taiju Tsuiki]

Created attachment 137863 [details]
Patch

[Taiju Tsuiki]

Comment on attachment 137842 [details]
Patch

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

>> LayoutTests/fast/filesystem/file-after-reload-crash.html:12
>> +var try_count = parseInt(sessionStorage['try']) || 0;
> 
> Might be better using simpler way like URL parameter rather than relying on other storage? (Also I'm not sure when sessionStorage expires in DRT)

Done.

[WebKit Review Bot]

Comment on attachment 137863 [details]
Patch

Attachment 137863 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12443097

New failing tests:
fast/filesystem/file-after-reload-crash.html

[WebKit Review Bot]

Created attachment 137865 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kinuko Yasuda]

Comment on attachment 137863 [details]
Patch

Style issues only.

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

> LayoutTests/fast/filesystem/file-after-reload-crash.html:14
> +  // should reach here without crash

Make comments look like sentences (starting with a capital, ending with a period).

> LayoutTests/fast/filesystem/file-after-reload-crash.html:22
> +  var err = 'Unknown';

nit: use non-abbreviated names if possible.

> LayoutTests/fast/filesystem/file-after-reload-crash.html:28
> +  }

nit: This may feel a bit too much for a non-expected result.

[Taiju Tsuiki]

Created attachment 137896 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 137896 [details]
Patch

LGTM, thanks.

[WebKit Review Bot]

Comment on attachment 137896 [details]
Patch

Clearing flags on attachment: 137896

Committed r114724: <http://trac.webkit.org/changeset/114724>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.