311618 - Crash [@ QDFlushPortBuffer()]

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Mark Mentovai]

We're seeing crashes in QDFlushPortBuffer frequently enough to be alarming. 
These were caused by bug 301774.  Pending flush timers should be killed when
ports go away.

Comment 1

[Mark Mentovai]

Attachment: Fix, unfortunately unusable

widget/src/cocoa can't link against gfx/src/mac, so this breaks builds that use
dynamic libraries.  It should work with static builds.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Housekeeping: according to Bugzilla rules, crashes should be at least critical
:-)  I assume this is also to be fixed by 1.0.

Comment 3

[Mark Mentovai]

Yup, this will be fixed soon.  The flush manager needs to be replumbed as an
XPCOM service.

Comment 4

[Simon Fraser [no longer active]]

I'm seeing this on a regular basis while browsing with Camino:
Oct  9 21:08:54 Manzanita
/Volumes/Zog/Development/projects/mozilla/branch_18/builds/camino_static/camino/build/Camino.app/Contents/MacOS/Camino:
CGContextFlush: invalid context
Oct  9 21:09:09 Manzanita
/Volumes/Zog/Development/projects/mozilla/branch_18/builds/camino_static/camino/build/Camino.app/Contents/MacOS/Camino:
CGContextFlush: invalid context

Comment 5

[Mark Mentovai]

Attachment: XPCOMify and away goes trouble down the drain

This helped break up a long plane ride.  Doing the flush manager as an XPCOM
service actually simplified the implementation, because the static global
singleton crap can go away.  It's replaced with XPCOM crap.  Other than that,
the fix here is the same as the unusable fix in the other patch, except this
one's usable.

Comment 6

[Simon Fraser [no longer active]]

I wonder if grovelling for a service in every call to TearDownView() is going to
hurt performance.

Comment 7

[Mark Mentovai]

If it makes a difference at all, it gets lost in the pageload noise.  Unpatched,
I did two runs at 416ms.  Patched, I had a run at 427 and a run at 411.

Comment 8

[Mike Pinkerton (not reading bugmail)]

+nsQDFlushManager::FlushPortBuffer(CGrafPtr aPort, RgnHandle aRegion)
+{
+  CreateOrGetPort(aPort)->FlushPortBuffer(aRegion);
+  return NS_OK;
+}

what happens if |aPort| isn't found in our list? Looks like this will crash. On
a related note, I assume |aPort| isn't allowed to be |NULL| either? You should
check for that and at least mention something in the header.

looks pretty good otherwise.

Comment 9

[Mark Mentovai]

(In reply to comment #8)
> what happens if |aPort| isn't found in our list? Looks like this will crash.

It's CreateOrGetPort(aPort).  If aPort isn't in the list, it's created and
pushed onto the list.  This is just existing code that's moved around in the
file to keep the interface implementation together.

> On
> a related note, I assume |aPort| isn't allowed to be |NULL| either? You should
> check for that and at least mention something in the header.

There's nothing in the code that won't work if aPort is NULL, except that the
eventual ::QDFlushPortBuffer call will crash because NULL isn't a valid port,
just like any other random value isn't.  FlushPortBuffer, the public entry
point, is only defined to work when aPort is something that can be flushed.  I
don't see the benefit to adding an ::IsValidPort call here to enforce this.

Comment 10

[Asa Dotzler [:asa]]

Mark, we're really, really late in the game for taking serious changes like
this. If this has a prayer of getting in, we're gonna need to hear a lot more
about why such a large change is needed, what's the risk in taking this kind of
change, and what kinds of testing and review went into assuring that it doesn't
regress anything.

Comment 11

[Mark Mentovai]

Asa, this is Camino-only, and it's *the* Camino topcrasher.  Further, it's not a
serious change, it's minor replumbing to turn a raw C++ static object into an
XPCOM service.

Comment 12

[Simon Fraser [no longer active]]

Comment on attachment 199043 [details] [diff] [review]
XPCOMify and away goes trouble down the drain

Asa: we have to take this patch for Camino. Without it, we're left with a
crasher that's a regression from earlier releases.

Comment 13

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 199043 [details] [diff] [review]
XPCOMify and away goes trouble down the drain

r=pink

Comment 14

[Mark Mentovai]

Checked in on the trunk
and
on the branch