Crash [@ QDFlushPortBuffer()]

RESOLVED FIXED in Camino1.0

Status

defect
--
critical
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: mark, Assigned: mark)

Tracking

(4 keywords)

Dependency tree / graph
Bug Flags:
blocking1.8rc1 +
camino1.0 +

Details

(crash signature, )

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

14 years ago
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.
Assignee

Comment 1

14 years ago
Posted patch Fix, unfortunately unusable (obsolete) — Splinter Review
widget/src/cocoa can't link against gfx/src/mac, so this breaks builds that use
dynamic libraries.  It should work with static builds.
Housekeeping: according to Bugzilla rules, crashes should be at least critical
:-)  I assume this is also to be fixed by 1.0.
Severity: major → critical
Target Milestone: --- → Camino1.0
Assignee

Comment 3

14 years ago
Yup, this will be fixed soon.  The flush manager needs to be replumbed as an
XPCOM service.
Status: NEW → ASSIGNED
Flags: camino1.0+
Flags: blocking1.8rc1?
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
Assignee

Comment 5

14 years ago
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.
Attachment #198884 - Attachment is obsolete: true
Attachment #199043 - Flags: superreview?(sfraser_bugs)
Attachment #199043 - Flags: review?(mikepinkerton)
I wonder if grovelling for a service in every call to TearDownView() is going to
hurt performance.
Assignee

Comment 7

14 years ago
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.
+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.
Assignee

Comment 9

14 years ago
(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.
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.
Assignee

Comment 11

14 years ago
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 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.
Attachment #199043 - Flags: superreview?(sfraser_bugs) → superreview+
Assignee

Updated

14 years ago
Keywords: regression

Updated

14 years ago
Attachment #199043 - Flags: approval1.8rc1+

Updated

14 years ago
Flags: blocking1.8rc1? → blocking1.8rc1+
Comment on attachment 199043 [details] [diff] [review]
XPCOMify and away goes trouble down the drain

r=pink
Attachment #199043 - Flags: review?(mikepinkerton) → review+
Assignee

Comment 14

14 years ago
Checked in on the trunk
and
on the branch
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Crash Signature: [@ QDFlushPortBuffer()]
You need to log in before you can comment on or make changes to this bug.