Bug 67510 - Crash can occur when doing a PlatformCAAnimation::copy() with no valueFunction
Summary: Crash can occur when doing a PlatformCAAnimation::copy() with no valueFunction
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Chris Marrin
URL:
Keywords: InRadar
Depends on: 68202
Blocks: 120901
  Show dependency treegraph
 
Reported: 2011-09-02 11:21 PDT by Chris Marrin
Modified: 2013-09-06 15:35 PDT (History)
5 users (show)

See Also:


Attachments
Patch with testcase (5.45 KB, patch)
2011-09-02 17:04 PDT, Chris Marrin
simon.fraser: review+
Details | Formatted Diff | Diff
Additional patch (1.57 KB, patch)
2011-09-08 08:52 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
2nd Patch (3.39 KB, patch)
2011-09-15 12:12 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
2nd Patch with vcproj file to avoid build issue (4.27 KB, patch)
2011-09-15 12:26 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch (7.34 KB, patch)
2011-09-15 16:14 PDT, Chris Marrin
aroben: review+
Details | Formatted Diff | Diff
Resubmission of patch to get ews bot to compile it (7.34 KB, patch)
2011-09-19 10:21 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch to fix build (7.09 KB, patch)
2011-09-19 13:47 PDT, Chris Marrin
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2011-09-02 11:21:00 PDT
Code inspection shows that doing the copy() when there is no valueFunction (e.g., doing a matrix animation) can cause a crash on Windows, using CACF. It will ask CACF for the valueFunction name of a null valueFunction and there are no null checks in place.
Comment 1 Chris Marrin 2011-09-02 11:35:30 PDT
I'm making a test case now to show the crash
Comment 2 Jeff Miller 2011-09-02 12:30:11 PDT
<rdar://problem/10033897>
Comment 3 Chris Marrin 2011-09-02 17:04:38 PDT
Created attachment 106226 [details]
Patch with testcase
Comment 4 Simon Fraser (smfr) 2011-09-02 17:09:35 PDT
Comment on attachment 106226 [details]
Patch with testcase

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

> LayoutTests/animations/pause-crash.html:38
> +        setTimeout(function() {
> +            document.getElementById('box1').style.webkitAnimationPlayState = "paused";
> +            setTimeout(function() {
> +                document.getElementById('box1').style.webkitAnimationPlayState = "running";
> +                setTimeout(function() {
> +                    document.getElementById('results').innerHTML = 'Did not crash, so PASSED';
> +                    if (window.layoutTestController)
> +                        layoutTestController.notifyDone();
> +                }, 100);
> +            }, 100);
> +        }, 100);

Seems like you could reduce these timeouts.
Comment 5 Chris Marrin 2011-09-05 06:58:34 PDT
Timeouts reduced to 50ms each
Comment 6 Chris Marrin 2011-09-05 07:02:03 PDT
Landed in http://trac.webkit.org/changeset/94471
Comment 7 Chris Marrin 2011-09-07 19:51:21 PDT
There is a still a crash later in the process of copying the animation. Patch coming soon
Comment 8 Chris Marrin 2011-09-08 08:52:21 PDT
Created attachment 106746 [details]
Additional patch
Comment 9 Adam Roben (:aroben) 2011-09-08 09:33:51 PDT
Comment on attachment 106746 [details]
Additional patch

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

> Source/WebCore/ChangeLog:8
> +        Another fix to take care of one last crash when running pause-crash.html.

pause-crash.html isn't crashing on the build.webkit.org bots. Do you know why? Do we need some other test?
Comment 10 Chris Marrin 2011-09-15 12:12:10 PDT
Created attachment 107526 [details]
2nd Patch
Comment 11 Chris Marrin 2011-09-15 12:26:19 PDT
Created attachment 107530 [details]
2nd Patch with vcproj file to avoid build issue
Comment 12 Simon Fraser (smfr) 2011-09-15 12:28:42 PDT
Comment on attachment 107530 [details]
2nd Patch with vcproj file to avoid build issue

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

> Source/WebCore/platform/graphics/ca/win/LayerChangesFlusher.cpp:90
> +    void* savedExceptionRegistration;
> +    __asm mov eax, FS:[0]
> +    __asm mov [savedExceptionRegistration], EAX
> +    __asm mov eax, 0xffffffff
> +    __asm mov FS:[0], EAX
> +
> +    LRESULT result = shared().hookFired(code, wParam, lParam);
> +
> +    // Restore the exception handler
> +    __asm mov EAX, [savedExceptionRegistration]
> +    __asm mov FS:[0], EAX

Can this goop be moved to inline helper functions, that perhaps can be shared with other files? It's pretty mysterious that we only need it in this one place.
Comment 13 WebKit Review Bot 2011-09-15 12:29:23 PDT
Attachment 107530 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:17:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/platform/graphics/ca/win/LayerChangesFlusher.cpp:82:  Extra space before [  [whitespace/braces] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Chris Marrin 2011-09-15 12:51:46 PDT
(In reply to comment #12)
> (From update of attachment 107530 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107530&action=review
> 
> > Source/WebCore/platform/graphics/ca/win/LayerChangesFlusher.cpp:90
> > +    void* savedExceptionRegistration;
> > +    __asm mov eax, FS:[0]
> > +    __asm mov [savedExceptionRegistration], EAX
> > +    __asm mov eax, 0xffffffff
> > +    __asm mov FS:[0], EAX
> > +
> > +    LRESULT result = shared().hookFired(code, wParam, lParam);
> > +
> > +    // Restore the exception handler
> > +    __asm mov EAX, [savedExceptionRegistration]
> > +    __asm mov FS:[0], EAX
> 
> Can this goop be moved to inline helper functions, that perhaps can be shared with other files? It's pretty mysterious that we only need it in this one place.

The only other place this sort of thing is done is in the equivalent Flusher in Safari. I didn't want to do it there because I don't know anything about that code and whether it might need the existing behavior. 

I can move it to a more common place. But I don't know where that would be. Adam?
Comment 15 Adam Roben (:aroben) 2011-09-15 14:17:24 PDT
Comment on attachment 107530 [details]
2nd Patch with vcproj file to avoid build issue

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

> Source/WebCore/platform/graphics/ca/win/LayerChangesFlusher.cpp:79
> +    // When a hook is called, Windows puts an __try/__except block around the call.
> +    // The exception handler then ignores system exceptions like invalid addresses
> +    // and null pointers. But we want to catch these and crash so we can generate
> +    // crashlogs for debugging the error. So for the duration of the call we replace
> +    // the Windows exception handler with 0xffffffff, which indicates that the
> +    // exception should not be handled but should crash instead.

It is awesome that this actually worked!

>>> Source/WebCore/platform/graphics/ca/win/LayerChangesFlusher.cpp:90
>>> +    __asm mov FS:[0], EAX
>> 
>> Can this goop be moved to inline helper functions, that perhaps can be shared with other files? It's pretty mysterious that we only need it in this one place.
> 
> The only other place this sort of thing is done is in the equivalent Flusher in Safari. I didn't want to do it there because I don't know anything about that code and whether it might need the existing behavior. 
> 
> I can move it to a more common place. But I don't know where that would be. Adam?

Hmm...Probably a new .h file in platform/win would be most appropriate. Maybe we should use RAII, too? Something like "class StructedExceptionHandlerSuppressor"?
Comment 16 Chris Marrin 2011-09-15 16:14:37 PDT
Created attachment 107559 [details]
Patch
Comment 17 WebKit Review Bot 2011-09-15 16:17:48 PDT
Attachment 107559 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/win/StructuredExceptionHandlerSupressor.h:49:  Extra space before [  [whitespace/braces] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Adam Roben (:aroben) 2011-09-15 16:26:22 PDT
Comment on attachment 107559 [details]
Patch

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

Very nice!

> Source/WebCore/platform/win/StructuredExceptionHandlerSupressor.h:51
> +        __asm mov eax, 0xffffffff
> +        __asm mov FS:[0], EAX

eax vs. EAX; which one? (Ditto on lines 48 and 49.)

> Source/WebCore/platform/win/StructuredExceptionHandlerSupressor.h:59
> +        __asm mov EAX, [m_savedExceptionRegistration]

Weird that you can read from but not write to member variables.
Comment 19 Chris Marrin 2011-09-15 16:50:59 PDT
Landed in http://trac.webkit.org/changeset/95243
Comment 20 Simon Fraser (smfr) 2011-09-15 17:32:58 PDT
This broke the Windows build, attempted fix in http://trac.webkit.org/changeset/95246
Comment 21 Chris Marrin 2011-09-19 09:43:11 PDT
Reopening to fix the Windows build breakage
Comment 22 Chris Marrin 2011-09-19 10:21:28 PDT
Created attachment 107878 [details]
Resubmission of patch to get ews bot to compile it
Comment 23 WebKit Review Bot 2011-09-19 10:24:16 PDT
Attachment 107878 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/win/StructuredExceptionHandlerSupressor.h:49:  Extra space before [  [whitespace/braces] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Chris Marrin 2011-09-19 13:47:17 PDT
Created attachment 107915 [details]
Patch to fix build
Comment 25 WebKit Review Bot 2011-09-19 13:50:11 PDT
Attachment 107915 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/win/StructuredExceptionHandlerSupressor.h:52:  Extra space before [  [whitespace/braces] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Adam Roben (:aroben) 2011-09-19 13:55:37 PDT
Comment on attachment 107915 [details]
Patch to fix build

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

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:27330
> +                <File
> +                    RelativePath="..\platform\win\StructuredExceptionHandlerSupressor.h"
> +                    >
> +                </File>

Indentation looks screwy here.

> Source/WebCore/platform/win/StructuredExceptionHandlerSupressor.h:45
> +        // Windows puts an __try/__except block around some calls, such as hooks.
> +        // The exception handler then ignores system exceptions like invalid addresses
> +        // and null pointers. This class can be used to remove this block and prevent
> +        // it from catching the exception. Typically this will cause the exception to crash 
> +        // which is often desirable to allow crashlogs to be recorded for debugging purposed.
> +        // While this class is in scope we replace the Windows exception handler with 0xffffffff, 
> +        // which indicates that the exception should not be handled.

It would be good to include a link to the article that explains the 0xffffffff and FS:[0] stuff.

>> Source/WebCore/platform/win/StructuredExceptionHandlerSupressor.h:52
>> +        __asm mov [registration], eax
> 
> Extra space before [  [whitespace/braces] [5]

You should file a bug about the style checker getting this wrong.
Comment 27 Chris Marrin 2011-09-19 14:34:01 PDT
(In reply to comment #26)
> (From update of attachment 107915 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107915&action=review
> 
> > Source/WebCore/WebCore.vcproj/WebCore.vcproj:27330
> > +                <File
> > +                    RelativePath="..\platform\win\StructuredExceptionHandlerSupressor.h"
> > +                    >
> > +                </File>
> 
> Indentation looks screwy here.

Looking at the vcproj file, the indentation matches all the files around it.

> 
> > Source/WebCore/platform/win/StructuredExceptionHandlerSupressor.h:45
> > +        // Windows puts an __try/__except block around some calls, such as hooks.
> > +        // The exception handler then ignores system exceptions like invalid addresses
> > +        // and null pointers. This class can be used to remove this block and prevent
> > +        // it from catching the exception. Typically this will cause the exception to crash 
> > +        // which is often desirable to allow crashlogs to be recorded for debugging purposed.
> > +        // While this class is in scope we replace the Windows exception handler with 0xffffffff, 
> > +        // which indicates that the exception should not be handled.
> 
> It would be good to include a link to the article that explains the 0xffffffff and FS:[0] stuff.

Will do

> 
> >> Source/WebCore/platform/win/StructuredExceptionHandlerSupressor.h:52
> >> +        __asm mov [registration], eax
> > 
> > Extra space before [  [whitespace/braces] [5]
> 
> You should file a bug about the style checker getting this wrong.

Will do
Comment 28 Chris Marrin 2011-09-19 14:37:35 PDT
Opened https://bugs.webkit.org/show_bug.cgi?id=68391 for the bogus style message in the Windows assembly line
Comment 29 Adam Roben (:aroben) 2011-09-19 14:48:44 PDT
Comment on attachment 107915 [details]
Patch to fix build

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

>>> Source/WebCore/WebCore.vcproj/WebCore.vcproj:27330
>>> +                </File>
>> 
>> Indentation looks screwy here.
> 
> Looking at the vcproj file, the indentation matches all the files around it.

It looks like you're using spaces while the rest of the file uses tabs.
Comment 30 Chris Marrin 2011-09-21 09:21:05 PDT
Landed in http://trac.webkit.org/changeset/95479 with build fix.