Closed
Bug 311618
Opened 19 years ago
Closed 19 years ago
Crash [@ QDFlushPortBuffer()]
Categories
(Camino Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Camino1.0
People
(Reporter: mark, Assigned: mark)
References
()
Details
(4 keywords)
Crash Data
Attachments
(1 file, 1 obsolete file)
22.89 KB,
patch
|
mikepinkerton
:
review+
sfraser_bugs
:
superreview+
asa
:
approval1.8rc1+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
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•19 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•19 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•19 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)
Comment 6•19 years ago
|
||
I wonder if grovelling for a service in every call to TearDownView() is going to
hurt performance.
Assignee | ||
Comment 7•19 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.
Comment 8•19 years ago
|
||
+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•19 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•19 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•19 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•19 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•19 years ago
|
Keywords: regression
Updated•19 years ago
|
Attachment #199043 -
Flags: approval1.8rc1+
Updated•19 years ago
|
Flags: blocking1.8rc1? → blocking1.8rc1+
Comment 13•19 years ago
|
||
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•19 years ago
|
||
Checked in on the trunk
and
on the branch
Updated•14 years ago
|
Crash Signature: [@ QDFlushPortBuffer()]
You need to log in
before you can comment on or make changes to this bug.
Description
•