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.
I'm making a test case now to show the crash
<rdar://problem/10033897>
Created attachment 106226 [details] Patch with testcase
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.
Timeouts reduced to 50ms each
Landed in http://trac.webkit.org/changeset/94471
There is a still a crash later in the process of copying the animation. Patch coming soon
Created attachment 106746 [details] Additional patch
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?
Created attachment 107526 [details] 2nd Patch
Created attachment 107530 [details] 2nd Patch with vcproj file to avoid build issue
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.
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.
(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 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"?
Created attachment 107559 [details] Patch
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 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.
Landed in http://trac.webkit.org/changeset/95243
This broke the Windows build, attempted fix in http://trac.webkit.org/changeset/95246
Reopening to fix the Windows build breakage
Created attachment 107878 [details] Resubmission of patch to get ews bot to compile it
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.
Created attachment 107915 [details] Patch to fix build
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 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.
(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
Opened https://bugs.webkit.org/show_bug.cgi?id=68391 for the bogus style message in the Windows assembly line
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.
Landed in http://trac.webkit.org/changeset/95479 with build fix.