Bug 13026 - incomplete render of menu (assertion failing in -[WebBaseNetscapePluginView saveAndSetNewPortStateForUpdate:])
Summary: incomplete render of menu (assertion failing in -[WebBaseNetscapePluginView s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Darin Adler
URL: http://www.law.stanford.edu/employers/
Keywords: InRadar, NeedsReduction
: 13244 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-03-09 12:09 PST by Alexander Romanovich
Modified: 2007-04-02 14:32 PDT (History)
5 users (show)

See Also:


Attachments
Change to QDPixelFormatFromCGBitmapInfo to handle kCGBitmapByteOrderDefault (1.66 KB, patch)
2007-03-13 17:08 PDT, Bruce Q. Hammond
darin: review-
Details | Formatted Diff | Diff
another cut at a fix (5.85 KB, patch)
2007-03-13 18:39 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Does the right thing with bitmaps that have kCGBitmapByteOrderDefault (2.57 KB, patch)
2007-03-26 19:23 PDT, Bruce Q. Hammond
darin: review-
Details | Formatted Diff | Diff
Stack trace (7.97 KB, text/plain)
2007-03-27 07:52 PDT, David Kilzer (:ddkilzer)
no flags Details
don't use offscreen code path if data pointer is 0 (18.25 KB, patch)
2007-03-28 07:54 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
new patch with additional fixes (24.24 KB, patch)
2007-03-28 09:31 PDT, Darin Adler
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Romanovich 2007-03-09 12:09:02 PST
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.
Comment 1 mitz 2007-03-09 12:18:54 PST
ASSERTION FAILED: pOffScreenGWorld && !err
(WebKit/Plugins/WebBaseNetscapePluginView.mm:422 -[WebBaseNetscapePluginView saveAndSetNewPortStateForUpdate:])

Comment 2 Mark Rowe (bdash) 2007-03-10 01:02:51 PST
The assertion failure being hit was added in r20042 by Bruce Q Hammond.  Bruce, perhaps you could shed some light on this?
Comment 3 mitz 2007-03-12 01:46:29 PDT
                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?
Comment 4 Mark Rowe (bdash) 2007-03-13 16:22:28 PDT
<rdar://problem/5061026>
Comment 5 Bruce Q. Hammond 2007-03-13 17:08:07 PDT
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 6 Darin Adler 2007-03-13 17:35:13 PDT
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 7 Darin Adler 2007-03-13 17:48:14 PDT
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.
Comment 8 Darin Adler 2007-03-13 17:53:04 PDT
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.
Comment 9 Darin Adler 2007-03-13 18:23:48 PDT
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.
Comment 10 Darin Adler 2007-03-13 18:27:21 PDT
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.
Comment 11 Darin Adler 2007-03-13 18:39:31 PDT
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.
Comment 12 Darin Adler 2007-03-20 09:39:44 PDT
Is this bug still reproducible?
Comment 13 Alexander Romanovich 2007-03-20 09:52:23 PDT
(In reply to comment #12)
> Is this bug still reproducible?
> 

Just downloaded and tried 20341. Still reproducible.
Comment 14 Bruce Q. Hammond 2007-03-26 19:23:38 PDT
Created attachment 13824 [details]
Does the right thing with bitmaps that have kCGBitmapByteOrderDefault

stylistic changes of patch of 13Mar07
Comment 15 David Kilzer (:ddkilzer) 2007-03-27 07:51:06 PDT
(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.

Comment 16 David Kilzer (:ddkilzer) 2007-03-27 07:52:43 PDT
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
Comment 17 David Kilzer (:ddkilzer) 2007-03-27 07:53:51 PDT
(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 18 Darin Adler 2007-03-27 09:41:57 PDT
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.
Comment 19 Darin Adler 2007-03-28 06:57:19 PDT
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.
Comment 20 Darin Adler 2007-03-28 07:11:43 PDT
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!
Comment 21 Darin Adler 2007-03-28 07:14:19 PDT
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.
Comment 22 Darin Adler 2007-03-28 07:19:48 PDT
(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 23 Darin Adler 2007-03-28 07:20:53 PDT
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
Comment 24 Darin Adler 2007-03-28 07:54:48 PDT
Created attachment 13843 [details]
don't use offscreen code path if data pointer is 0
Comment 25 Darin Adler 2007-03-28 09:31:15 PDT
Created attachment 13844 [details]
new patch with additional fixes
Comment 26 Darin Adler 2007-03-28 18:32:02 PDT
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.
Comment 27 mitz 2007-03-31 07:50:47 PDT
*** Bug 13244 has been marked as a duplicate of this bug. ***
Comment 28 Geoffrey Garen 2007-04-02 12:50:17 PDT
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
Comment 29 Darin Adler 2007-04-02 12:57:32 PDT
(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.
Comment 30 Darin Adler 2007-04-02 14:32:46 PDT
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.