Bug 76461 - DOMFileSystem::scheduleCallback() crashes with null scriptExecutionContext().
Summary: DOMFileSystem::scheduleCallback() crashes with null scriptExecutionContext().
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Taiju Tsuiki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-17 09:11 PST by Taiju Tsuiki
Modified: 2012-04-20 00:40 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.43 KB, patch)
2012-01-17 09:17 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (1.62 KB, patch)
2012-02-02 00:27 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (4.45 KB, patch)
2012-04-18 22:48 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (4.48 KB, patch)
2012-04-18 22:52 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (4.37 KB, patch)
2012-04-19 01:57 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (5.86 MB, application/zip)
2012-04-19 02:30 PDT, WebKit Review Bot
no flags Details
Patch (4.39 KB, patch)
2012-04-19 07:01 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Taiju Tsuiki 2012-01-17 09:11:40 PST
Referring http://crbug.com/94895, WebKit crashes at the point.
I tried to fix the bug with the following patch.
Comment 1 Taiju Tsuiki 2012-01-17 09:17:18 PST
Created attachment 122776 [details]
Patch
Comment 2 Darin Adler 2012-01-17 17:54:59 PST
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.
Comment 3 Taiju Tsuiki 2012-02-02 00:27:51 PST
Created attachment 125090 [details]
Patch
Comment 4 Taiju Tsuiki 2012-02-02 00:28:27 PST
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.
Comment 5 Taiju Tsuiki 2012-03-08 09:41:17 PST
Could you take another look?
Comment 6 Kinuko Yasuda 2012-04-17 23:33:29 PDT
This patch looks good to me.
Comment 7 Kinuko Yasuda 2012-04-17 23:35:08 PDT
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;
Comment 8 David Levin 2012-04-18 10:22:49 PDT
Comment on attachment 125090 [details]
Patch

What Kinuko said.
Comment 9 Taiju Tsuiki 2012-04-18 22:48:13 PDT
Created attachment 137840 [details]
Patch
Comment 10 Taiju Tsuiki 2012-04-18 22:52:36 PDT
Created attachment 137842 [details]
Patch
Comment 11 Taiju Tsuiki 2012-04-18 22:55:58 PDT
I fixed the style and added a test.
Could you take another look?
Comment 12 Kinuko Yasuda 2012-04-18 23:47:52 PDT
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)
Comment 13 Taiju Tsuiki 2012-04-19 01:57:41 PDT
Created attachment 137863 [details]
Patch
Comment 14 Taiju Tsuiki 2012-04-19 01:59:46 PDT
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.
Comment 15 WebKit Review Bot 2012-04-19 02:30:16 PDT
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
Comment 16 WebKit Review Bot 2012-04-19 02:30:21 PDT
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
Comment 17 Kinuko Yasuda 2012-04-19 02:36:33 PDT
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.
Comment 18 Taiju Tsuiki 2012-04-19 07:01:51 PDT
Created attachment 137896 [details]
Patch
Comment 19 Kinuko Yasuda 2012-04-19 23:16:55 PDT
Comment on attachment 137896 [details]
Patch

LGTM, thanks.
Comment 20 WebKit Review Bot 2012-04-20 00:40:30 PDT
Comment on attachment 137896 [details]
Patch

Clearing flags on attachment: 137896

Committed r114724: <http://trac.webkit.org/changeset/114724>
Comment 21 WebKit Review Bot 2012-04-20 00:40:43 PDT
All reviewed patches have been landed.  Closing bug.