If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Crash [@ QDFlushPortBuffer()]

RESOLVED FIXED in Camino1.0

Status

Camino Graveyard
General
--
critical
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Mark Mentovai, Assigned: Mark Mentovai)

Tracking

(4 keywords)

1.8 Branch
Camino1.0
PowerPC
Mac OS X
crash, fixed1.8, regression, topcrash
Bug Flags:
blocking1.8rc1 +
camino1.0 +

Details

(crash signature, URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 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

12 years ago
Created attachment 198884 [details] [diff] [review]
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.
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

12 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?

Comment 4

12 years ago
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

12 years ago
Created attachment 199043 [details] [diff] [review]
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.
Attachment #198884 - Attachment is obsolete: true
Attachment #199043 - Flags: superreview?(sfraser_bugs)
Attachment #199043 - Flags: review?(mikepinkerton)

Comment 6

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

Comment 7

12 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

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

Comment 10

12 years ago
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

12 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 12

12 years ago
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

12 years ago
Keywords: regression

Updated

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

Updated

12 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

12 years ago
Checked in on the trunk
and
on the branch
Status: ASSIGNED → RESOLVED
Last Resolved: 12 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.