Bug 8425 – opacity on a group containing a blur filtered rect crashes

• Component: SVG
• Status: RESOLVED FIXED

[Alexander Kellett]

opacity on a group containing a blur filtered rect crashes

[Alexander Kellett]

Created attachment 7748 [details]
reduced testcase

[Eric Seidel (no email)]

This is probably caused by my bogus use of +[NSGraphicsContext setCurrentContext:].  Using opactiy will cause the parent to open a transparency layer.  Using a filter will cause the child to try and swap out the current context with setCurrentContext while the transparency layer is still open.  That's probably a bad thing :)  And probably what is causing the crash.

[Alexey Proskuryakov]

Reproducible crash -> P1.

[Alice Liu]

Date/Time:      2006-07-11 19:03:14.595 -0700
OS Version:     10.4.7 (Build 8J135)
Report Version: 4

Command: Safari
Path:    /Build/symroots/Debug/Safari.app/Contents/MacOS/Safari
Parent:  tcsh [219]

Version: 3.0 (521.15)

PID:    8003
Thread: 0

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0x7f80000f

Thread 0 Crashed:
0   libSystem.B.dylib        	0x9000197c pthread_mutex_lock + 36
1   <<00000000>> 	0x00000000 0 + 0
2   com.apple.CoreGraphics   	0x903bcf28 CGColorTransformGetColorSpace + 40
3   libRIP.A.dylib           	0x9470d018 ripc_GetRenderingState + 88
4   libRIP.A.dylib           	0x9470c1a4 ripc_DrawImage + 152
5   com.apple.CoreGraphics   	0x903d18e8 CGContextDelegateDrawImage + 76
6   com.apple.CoreGraphics   	0x904635a0 metalDelegate_FillRectWithImages + 360
7   com.apple.CoreGraphics   	0x90463420 CGContextDelegateFillRectWithImages + 116
8   com.apple.CoreGraphics   	0x90463204 Private_CGContextFillRectWithImagesPrivate + 124
9   com.apple.HIToolbox      	0x934dadd8 HIThemeDrawMetalWithProvider(CGRect const*, CGContext*) + 224
10  com.apple.HIToolbox      	0x9325a344 HIThemeDrawBackground + 220
11  com.apple.AppKit         	0x937fff80 -[NSGrayFrame drawWindowBackgroundRegion:level:] + 340
12  com.apple.AppKit         	0x937534a8 -[NSFrameView drawThemeContentFill:inView:] + 1028
13  com.apple.AppKit         	0x937ffd3c -[NSGrayFrame drawRect:] + 48
14  com.apple.AppKit         	0x9372f858 -[NSView _drawRect:clip:] + 2128
15  com.apple.AppKit         	0x9372e5fc -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] + 736
16  com.apple.AppKit         	0x9374f044 -[NSThemeFrame _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] + 192
17  com.apple.AppKit         	0x93728054 -[NSView _displayRectIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:] + 384
18  com.apple.AppKit         	0x9371d348 -[NSView displayIfNeeded] + 248
19  com.apple.AppKit         	0x9371d1b8 -[NSWindow displayIfNeeded] + 180
20  com.apple.Safari         	0x000578d4 -[BrowserWindow displayIfNeeded] + 328 (BrowserWindow.m:301)
21  com.apple.AppKit         	0x9371d064 _handleWindowNeedsDisplay + 200
22  com.apple.CoreFoundation 	0x907db73c __CFRunLoopDoObservers + 352
23  com.apple.CoreFoundation 	0x907db9dc __CFRunLoopRun + 420
24  com.apple.CoreFoundation 	0x907db47c CFRunLoopRunSpecific + 268
25  com.apple.HIToolbox      	0x931e6740 RunCurrentEventLoopInMode + 264
26  com.apple.HIToolbox      	0x931e5dd4 ReceiveNextEventCommon + 380
27  com.apple.HIToolbox      	0x931e5c40 BlockUntilNextEventMatchingListInMode + 96
28  com.apple.AppKit         	0x936e9ae4 _DPSNextEvent + 384
29  com.apple.AppKit         	0x936e97a8 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 116
30  com.apple.Safari         	0x000322b4 -[BrowserApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 292 (BrowserApplication.m:166)
31  com.apple.AppKit         	0x936e5cec -[NSApplication run] + 472
32  com.apple.AppKit         	0x937d687c NSApplicationMain + 452
33  com.apple.Safari         	0x00101de8 main + 420 (main.m:36)
34  com.apple.Safari         	0x0000265c _start + 340 (crt.c:272)
35  com.apple.Safari         	0x00002504 start + 60

[Dave MacLachlan]

I'm not sure if it helps, but you can avoid the crash by not releasing the m_filterCIContext from the Filter in kCanvasFilterQuartz::applyFilter (approximately line 154 in kCanvasFilterQuarts.mm). Interestingly when you create the filter, the CGContext you pass in gets it's retain count incremented, but when you release the filter, the CGContext it wraps doesn't appear to get it's retain count decremented. 


CGLayerRelease(m_filterCGLayer);
m_filterCGLayer = 0;

->HardRelease(m_filterCIContext); //Remove this line and things seem fine...
m_filterCIContext = 0;

Also if you skip over the CGBegin/EndTransparencyGroup calls everything appears alright as well. Bugs with doing filters in transparency groups perhaps?

calling and restoring the context in a transparencygroup doesn't seem to be the problem, I wrote up quite a bit of sample test code and couldn't repro.

[Dave MacLachlan]

Created attachment 9619 [details]
Fix for 8425 and by extension 6947

[Eric Seidel (no email)]

Comment on attachment 9619 [details]
Fix for 8425 and by extension 6947

Looks great!  The only thing it needs is a changelog and your copyright.  Without those however, this can't be landed.

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> (From update of attachment 9619 [details] [edit])
> Looks great!  The only thing it needs is a changelog and your copyright. 
> Without those however, this can't be landed.

Create a ChangeLog entry using the WebKitTools/Scripts/prepare-ChangeLog script.

[Dave MacLachlan]

Created attachment 9630 [details]
Fix for 8425 and by extension 6947

This is the updated patch with ChangeLog and copyright notice attached. Thanks!

[Darin Adler]

Comment on attachment 9630 [details]
Fix for 8425 and by extension 6947

But also, it's better to just do this with API instead of SPI. Just create and use an autorelease pool in the right place.
    NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
    m_filterCIContext = HardRetain([CIContext contextWithCGContext:cgContext options:contextOptions]);
    [pool drain];

KCanvasFilterQuartz::prepareFilter shhould not use this SPI either -- same technique can be used there.

[Dave MacLachlan]

Created attachment 9641 [details]
Fix for 8425 and by extension 6947

Darin...good catch. Fixed it up how you suggested.

[Darin Adler]

Comment on attachment 9641 [details]
Fix for 8425 and by extension 6947

r=me

Comment is a little long, but the fix is good.

[Alexey Proskuryakov]

A few nitpicks: the test should ideally include pass criteria ("should not crash"), display something green to make success immediately obvious, and have a link to this Bugzilla issue. Also, onload on <svg> seems unnecessary, unless I'm missing something.

Neither of these is important enough to prevent landing, but since we're "waiting for the floodgates to open" anyway...

I'm also not quite sure about using -[NSAutoreleasePool release] here - if the goal is to manually control deallocation, isn't drain the right choice under GC?

[Dave MacLachlan]

Created attachment 9652 [details]
Fix for 8425 and by extension 6947

New patch fixes up points raised by Alexey and Darin.
Filed Radar describing CGEndTransparencyGroup badness
<rdar://problem/4647735>

[Alexey Proskuryakov]

Comment on attachment 9652 [details]
Fix for 8425 and by extension 6947

r=me

[Alexey Proskuryakov]

Committed revision 15603.