Summary: | incomplete render of menu (assertion failing in -[WebBaseNetscapePluginView saveAndSetNewPortStateForUpdate:]) | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Romanovich <alex> | ||||||||||||||
Component: | Plug-ins | Assignee: | Darin Adler <darin> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | bruceq, ddkilzer, martin_j, mitz, mrowe | ||||||||||||||
Priority: | P1 | Keywords: | InRadar, NeedsReduction | ||||||||||||||
Version: | 523.x (Safari 3) | ||||||||||||||||
Hardware: | Mac | ||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||
URL: | http://www.law.stanford.edu/employers/ | ||||||||||||||||
Attachments: |
|
Description
Alexander Romanovich
2007-03-09 12:09:02 PST
ASSERTION FAILED: pOffScreenGWorld && !err (WebKit/Plugins/WebBaseNetscapePluginView.mm:422 -[WebBaseNetscapePluginView saveAndSetNewPortStateForUpdate:]) The assertion failure being hit was added in r20042 by Bruce Q Hammond. Bruce, perhaps you could shed some light on this? if (pixelFormat == 0) { // Not a valid pixel format - don't render at all. offscreenBounds.top = 0; offscreenBounds.left = 0; offscreenBounds.right = 0; offscreenBounds.bottom = 0; rowBytes = 0; pixelFormat = k32BGRAPixelFormat; } These parameters are invalid for NewGWorldFromPtr, which returns a -50 error. Not sure that's the best way to accomplish "don't render at all", even if this code is not supposed to be reached. In the test case, the 0 pixelFormat is what QDPixelFormatFromCGBitmapInfo() returns for kCGBitmapByteOrderDefault. Is there a way to resolve kCGBitmapByteOrderDefault in compile time? Created attachment 13619 [details]
Change to QDPixelFormatFromCGBitmapInfo to handle kCGBitmapByteOrderDefault
QDPixelFormatFromCGBitmapInfo will now do something reasonable
with bitmaps that have byte order of kCGBitmapByteOrderDefault.
Comment on attachment 13619 [details]
Change to QDPixelFormatFromCGBitmapInfo to handle kCGBitmapByteOrderDefault
r=me -- we need this fix!
Why did you change the indentation? Why are the static_cast calls needed?
Comment on attachment 13619 [details]
Change to QDPixelFormatFromCGBitmapInfo to handle kCGBitmapByteOrderDefault
We don't really want to link to libkern just to find out the endianness. We don't want to keep the dead code to handle 0 either. I'll investigate a little myself.
Why are we exercising the bitmap context code path at all? I thought that code path wouldn't be used at all for Safari. Is "something reasonable" really correct? We need to get the format right, not just reasonable, I imagine. This code was not supposed to be exercised at all in Safari. Do you know why it's taking this code path? Maybe we need an additional check to see if the graphics context is the same as the -[NSWindow graphicsContext]. If it is, then I don't think we want to go into the bitmap context code path. I'll attach a patch in a bit, with my proposed change. I am annoyed, though, that we don't have a suitable test. In my testing, I can't reproduce a case where we go through the BitmapContext code path, this I can't reproduce the bug either. Created attachment 13623 [details]
another cut at a fix
But I still don't understand why this code path is exercised at all in Safari. I can't reproduce a case where it's a BitmapContext, but it seems like others can.
Is this bug still reproducible? (In reply to comment #12) > Is this bug still reproducible? > Just downloaded and tried 20341. Still reproducible. Created attachment 13824 [details]
Does the right thing with bitmaps that have kCGBitmapByteOrderDefault
stylistic changes of patch of 13Mar07
(In reply to comment #10) > In my testing, I can't reproduce a case where we go through the BitmapContext > code path, this I can't reproduce the bug either. I was able to reproduce the bug simply by opening the web page and then hovering over the menu. At the time, CPU usage was near 100% though because I was rebuilding a debug build of WebKit. (The menu didn't even open before the assertion was hit.) Also, I'm using a PowerBook G4 (not an Intel Mac) if that makes any difference. Created attachment 13828 [details]
Stack trace
Console output when assertion is hit:
ASSERTION FAILED: pOffScreenGWorld && !err
(/path/to/WebKit/WebKit/Plugins/WebBaseNetscapePluginView.mm:423 -[WebBaseNetscapePluginView saveAndSetNewPortStateForUpdate:])
Segmentation fault
(In reply to comment #16) > Created an attachment (id=13828) [edit] > Stack trace Using a local debug build of WebKit r20504 with Safari 2.0.4 (419.3) on Mac OS X 10.4.9 (8P135). Comment on attachment 13824 [details]
Does the right thing with bitmaps that have kCGBitmapByteOrderDefault
This patch still doesn't answer my question. Why is any of this code running at all in Safari? Where is the CGBitmapContext coming from? Until I have an answer to that I'm not comfortable saying review+.
And I would prefer not to have a runtime check to figure out the byte order, when there's a perfectly good compile time check already done in the CoreGraphics headers. Please look at my patch for an alternative that does not involve a call to a function and also makes fewer assumptions and may be more correct.
OK, I figured out why the code is unexpectedly running inside Safari. The plug-in is inside a transparency layer. When drawing inside a transparency layer, the current GraphicsContext is going to be a CGBitmapContext. One that was created with CGContextBeginTransparencyLayer. But I think we don't want to use the special code path in that case. The real bug here is that WKCGContextIsBitmapContext is returning true, but the context is *not* a bitmap context. For example, CGBitmapContextGetData returns 0 and CGBitmapContextGetBitmapInfo returns all zeros! Turns out that we don't need the WebKitSystemInterface call at all. Just calling CGBitmapContextGetData checks if the current context is a bitmap context, without doing any special logging when it's not. As far as I can tell from my testing. (In reply to comment #21) > Turns out that we don't need the WebKitSystemInterface call at all. Just > calling CGBitmapContextGetData checks if the current context is a bitmap > context, without doing any special logging when it's not. As far as I can tell > from my testing. That's not quite true. But it is true that in the layer case, we get zero from the bitmap context data pointer. So this gives us an excellent way to detect that case and handle it differently. I'll put together a patch now. Comment on attachment 13824 [details]
Does the right thing with bitmaps that have kCGBitmapByteOrderDefault
review-, because the issue isn't just the bitmap info -- it's also the data pointer
Created attachment 13843 [details]
don't use offscreen code path if data pointer is 0
Created attachment 13844 [details]
new patch with additional fixes
Comment on attachment 13844 [details]
new patch with additional fixes
I noticed that one of the two bugs has the wrong description in the change log. I'll fix that when landing.
*** Bug 13244 has been marked as a duplicate of this bug. *** Comment on attachment 13844 [details]
new patch with additional fixes
+ CGContextRef context = (CGContextRef)[[NSGraphicsContext currentContext] graphicsPort];
I'm curious -- why did you choose the C-style cast here?
r=me
(In reply to comment #28) > (From update of attachment 13844 [details] [edit]) > + CGContextRef context = (CGContextRef)[[NSGraphicsContext > currentContext] graphicsPort]; > > I'm curious -- why did you choose the C-style cast here? I think it's the heritage of this file. It used to be a C source file and was only recently changed to C++. I think I copied and pasted it from elsewhere in the file. Sadly, I couldn't come up with a good way to do regression tests. Sending WebKit/ChangeLog Sending WebKit/Plugins/WebBaseNetscapePluginView.h Sending WebKit/Plugins/WebBaseNetscapePluginView.mm Transmitting file data ... Committed revision 20668. |