Bug 67510 – Crash can occur when doing a PlatformCAAnimation::copy() with no valueFunction

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Chris Marrin]

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.

[Chris Marrin]

I'm making a test case now to show the crash

[Jeff Miller]

<rdar://problem/10033897>

[Chris Marrin]

Created attachment 106226 [details]
Patch with testcase

[Simon Fraser (smfr)]

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.

[Chris Marrin]

Timeouts reduced to 50ms each

[Chris Marrin]

Landed in http://trac.webkit.org/changeset/94471

[Chris Marrin]

There is a still a crash later in the process of copying the animation. Patch coming soon

[Chris Marrin]

Created attachment 106746 [details]
Additional patch

[Adam Roben (:aroben)]

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?

[Chris Marrin]

Created attachment 107526 [details]
2nd Patch

[Chris Marrin]

Created attachment 107530 [details]
2nd Patch with vcproj file to avoid build issue

[Simon Fraser (smfr)]

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.

[WebKit Review Bot]

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.

[Chris Marrin]

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

[Adam Roben (:aroben)]

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"?

[Chris Marrin]

Created attachment 107559 [details]
Patch

[WebKit Review Bot]

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.

[Adam Roben (:aroben)]

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.

[Chris Marrin]

Landed in http://trac.webkit.org/changeset/95243

[Simon Fraser (smfr)]

This broke the Windows build, attempted fix in http://trac.webkit.org/changeset/95246

[Chris Marrin]

Reopening to fix the Windows build breakage

[Chris Marrin]

Created attachment 107878 [details]
Resubmission of patch to get ews bot to compile it

[WebKit Review Bot]

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.

[Chris Marrin]

Created attachment 107915 [details]
Patch to fix build

[WebKit Review Bot]

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.

[Adam Roben (:aroben)]

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.

[Chris Marrin]

(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

[Chris Marrin]

Opened https://bugs.webkit.org/show_bug.cgi?id=68391 for the bogus style message in the Windows assembly line

[Adam Roben (:aroben)]

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.

[Chris Marrin]

Landed in http://trac.webkit.org/changeset/95479 with build fix.