Bug 13026 – incomplete render of menu (assertion failing in -[WebBaseNetscapePluginView saveAndSetNewPortStateForUpdate:])

• Component: Plug-ins
• Status: RESOLVED FIXED

[Alexander Romanovich]

The Stanford Law site has an "information for" menu that renders with gaps on current release of Safari. Using 20057 I can reproduce it as well, although the menu is far less broken. Mouse over the menu and drag down slowly over the menu items to see the glitch.

ASSERTION FAILED: pOffScreenGWorld && !err
(WebKit/Plugins/WebBaseNetscapePluginView.mm:422 -[WebBaseNetscapePluginView saveAndSetNewPortStateForUpdate:])

[Mark Rowe (bdash)]

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?

[Mark Rowe (bdash)]

<rdar://problem/5061026>

[Bruce Q. Hammond]

Created attachment 13619 [details]
Change to QDPixelFormatFromCGBitmapInfo to handle kCGBitmapByteOrderDefault

QDPixelFormatFromCGBitmapInfo will now do something reasonable
with bitmaps that have byte order of kCGBitmapByteOrderDefault.

[Darin Adler]

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?

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

Is this bug still reproducible?

[Alexander Romanovich]

(In reply to comment #12)
> Is this bug still reproducible?
> 

Just downloaded and tried 20341. Still reproducible.

[Bruce Q. Hammond]

Created attachment 13824 [details]
Does the right thing with bitmaps that have kCGBitmapByteOrderDefault

stylistic changes of patch of 13Mar07

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

(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).

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

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!

[Darin Adler]

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.

[Darin Adler]

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

[Darin Adler]

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

[Darin Adler]

Created attachment 13843 [details]
don't use offscreen code path if data pointer is 0

[Darin Adler]

Created attachment 13844 [details]
new patch with additional fixes

[Darin Adler]

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. ***

[Geoffrey Garen]

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

[Darin Adler]

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

[Darin Adler]

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.