[10.4] Tiger: Crash at [NSQuickDrawView dealloc]

RESOLVED FIXED in Camino0.9

Status

Camino Graveyard
General
P1
critical
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Avi Drissman, Assigned: Simon Fraser)

Tracking

({crash})

unspecified
Camino0.9
PowerPC
Mac OS X
crash

Details

Attachments

(3 attachments, 1 obsolete attachment)

21.77 KB, text/plain
Details
21.48 KB, text/plain
Details
2.38 KB, patch
Mike Pinkerton (not reading bugmail)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
Version 2005061608 (0.9a1)

I'm getting fairly constant crashes that look like:

0   com.apple.QD                   	0x916acc28 SetGWorld + 64
1   com.apple.AppKit               	0x939cf030 -[NSQuickDrawView dealloc] + 36
2   org.mozilla.navigator          	0x00604b38 -[ChildView dealloc] + 44
3   com.apple.AppKit               	0x9362dacc -[NSView release] + 200
4   com.apple.Foundation           	0x92886f1c __delayedPerformCleanup + 48
5   com.apple.CoreFoundation       	0x90771470 CFRunLoopTimerInvalidate + 400
6   com.apple.CoreFoundation       	0x9075df08 __CFRunLoopDoTimer + 272

This has happened several times now on different sites; see talkbacks TB6754097G
and TB6816094M.

The stack traces seem very similar to those from bug 198101 which was closed
quite a while back.
(Reporter)

Comment 1

13 years ago
Oh, yeah. Tiger, 10.4.1.
If this bug needed to be confirmed, I would do so. It looks like I've gotten it
as well, see TB6818265Y. I'm using OS X 10.4.1 and 2005061708.
Keywords: crash
(Reporter)

Comment 3

13 years ago
Back in bug 198101, it was noted that [NSQuickDrawView dealloc] restored the
previously-active port, and that Camino, in its subclass, immediately called
SetPort(NULL) after, to make sure that no bad port stayed set. It seems now that
you crash as soon as you SetPort (or the equivalent here, SetGWorld) a bad port.
Perhaps this is new in Tiger? Should we cc Joseph Maurer, who's Mr. QuickDraw at
Apple, to see if this is indeed the case?
(Assignee)

Updated

13 years ago
Priority: -- → P1
Summary: Crash at [NSQuickDrawView dealloc] → [10.4] Tiger: Crash at [NSQuickDrawView dealloc]
Target Milestone: --- → Camino0.9
(Assignee)

Comment 4

13 years ago
*** Bug 297613 has been marked as a duplicate of this bug. ***

Comment 5

13 years ago
Created attachment 186919 [details]
Typical Crash Log from the 0.9a1 alpha release

I get this crash a lot too. I'm attaching a Crash Log from the 0.9a1 alpha
release.
whee i saw this today as well.
Assignee: pinkerton → sfraser_bugs

Comment 7

13 years ago
Created attachment 187382 [details]
Crash Log from Camino 0.9 (typical)

Forgive me if I am doing this incorrectly but I am new to this.... I cannot use
Camino 0.9 because of frequent crashes, and, if I understand this system, it
might be helpful to squashing the bug if I post a crash log here. I've returned
to 0.8.4 since the alpha is so unstable. How does one learn when this bug has
been fixed? Thank you for your patience with me as a newbie!

Comment 8

13 years ago
In dealloc for ChildView:

[super dealloc];    // this sets the current port to _savePort
SetPort(NULL);      // this is safe on OS X; it will set the port to
                            // an empty fallback port.

Looks like this isn't safe any more...
(Assignee)

Comment 9

13 years ago
I think(In reply to comment #8)
> Looks like this isn't safe any more...

I think that's unlikely; I doubt they'd change such a behaviour.

If you look at the stack, it's crashing in SetGWorld() called from
-[NSQuickDrawView dealloc], not -[ChildView dealloc], so that SetPort(NULL) is
not the one that crashes.

It's notable that CFRunLoopTimerInvalidate() is on the stack. I wonder if my adding
  [mView performSelector:@selector(setNeedsDisplayWithValue:) withObject:nil
afterDelay:0];
caused this.
Status: NEW → ASSIGNED
my gf is running a build (on tiger) from a couple months ago and hasn't seen
this problem at all. i think it's a "recent" regresion.
(Assignee)

Comment 11

13 years ago
(In reply to comment #10)
> my gf is running a build (on tiger) from a couple months ago and hasn't seen
> this problem at all. i think it's a "recent" regresion.

Like from 6/16, when bug 294415 was checked in?

Comment 12

13 years ago
(In reply to comments #8, #9 and  #10)

The problem seems to be that [NSQuickDrawView dealloc] restores (via SetGWorld) whatever the current 
GWorld/GDevice was (some instance variable "savedPort") when the QuickdrawView was created, and that 
this previously current GWorld/GDevice either already was invalid (deallocated), or has been deallocated in 
the meantime.
The crash occurs or not, depending on whether the memory occupied by the now deallocated GWorld has 
been overwritten or not. Because of a change in the allocation patterns from one system revision to 
another, it may appear as a regression, but probably is not (I don't know what has changed in 
NSQuickdrawView, if anything).
SetPort(NULL) is always safe; but it just doesn't address the real problem. 
(Assignee)

Comment 13

13 years ago
(In reply to comment #12)
> (In reply to comments #8, #9 and  #10)
> 
> The problem seems to be that [NSQuickDrawView dealloc] restores (via
> SetGWorld) whatever the current GWorld/GDevice was (some instance
> variable "savedPort") when the QuickdrawView was created, and that
> this previously current GWorld/GDevice either already was invalid
> (deallocated), or has been deallocated in the meantime.

Right. In other words, NSQuickDrawView expects to be used in a very constrained,
"nested" way with a well-specified lifetime. With autoreleasing and such, I
don't see how this can be expected in any Cocoa app, let alone one as complex as
a web browser.

> The crash occurs or not, depending on whether the memory occupied by
> the now deallocated GWorld has been overwritten or not. Because of a
> change in the allocation patterns from one system revision to
> another, it may appear as a regression, but probably is not (I don't
> know what has changed in NSQuickdrawView, if anything). SetPort(NULL)
> is always safe; but it just doesn't address the real problem. 

So would it be sensible to do a SetPort(NULL) just *before* our NSQuickDrawView
subclass calls -[NSQuickDrawView init], so that NSQuickDrawView stashes a "known
good" GWorld?

Comment 14

13 years ago
(In reply to comment #13)

Yes. (Should have suggested it myself :-).
(Assignee)

Comment 15

13 years ago
Created attachment 187612 [details] [diff] [review]
Patch

This patch adds a call to SetPort(NULL) before the initialization of
ChildView's NSQuickDrawView superclass. It also changes the code to call the
designated initializer (-initWithFrame:) of NSQuickDrawView by passing in the
frame.

Can someone test this on Tiger?
Attachment #187612 - Flags: review?(pinkerton)
Comment on attachment 187612 [details] [diff] [review]
Patch

-  [mView setFrame:r];
+  //[mView setFrame:r];

shouldn't we just take this out?

looks good r=pink.
Attachment #187612 - Flags: review?(pinkerton) → review+

Comment 17

13 years ago
landed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 18

13 years ago
I'm still getting this crash with the 20050703 build. Twice within 30 minutes.

Comment 19

13 years ago
Same here. One of the Talkback IDs is TB7244415G.
(Assignee)

Comment 20

13 years ago
Joseph: can you tell us anything about the "safe" port that is used when
SetPort() is called with NULL? Is it tied to a WindowPtr? Maybe we should be
making our own little GWorld to use as a "safe" port?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 21

13 years ago
(In reply to comment #20)

No, it's not tied to any window. It's what you get back from CreateNewPort(), but the with an empty 
bounds rectangle in the portPixMap. - It's definitely not worth trying to use your own fallback GWorld 
instead.
Given that the crash persists, something else must be going on. 
(Assignee)

Comment 22

13 years ago
Created attachment 188401 [details] [diff] [review]
Patch

This patch essentially moves the SetPort(NULL) from -initWithFrame to
-lockFocus.

I determined with a bunch of printfs() and calls to IsValidPort() that
NSQuickDrawView doesn't actually assign to _savePort until -lockFocus is
called, so the previous patch wasn't doing much at all.

This patch also adds calls to |super| on a few methods that should have them
(just in case NSQuickDrawView implementes them), and adds an assertion that
should fire in the case where we may crash.
(Assignee)

Updated

13 years ago
Attachment #187612 - Attachment is obsolete: true
Attachment #188401 - Flags: review?(pinkerton)
+- (void)lockFocus
+{
...
+  SetPort(NULL);
+  [super lockFocus];

Do we want to do this every time, or only the first time we draw/lockFocus?
Seems like it might cause other hidden issues to always be changing the port. Or
maybe we just want to be uber-safe.
(Assignee)

Comment 24

13 years ago
(In reply to comment #23)
> +- (void)lockFocus
> +{
> ...
> +  SetPort(NULL);
> +  [super lockFocus];
> 
> Do we want to do this every time, or only the first time we draw/lockFocus?
> Seems like it might cause other hidden issues to always be changing the port. Or
> maybe we just want to be uber-safe.

We have to do it every time, because NSQDView stashes the current port in
_savePort every time.
Status: REOPENED → ASSIGNED
Comment on attachment 188401 [details] [diff] [review]
Patch

r=pink
Attachment #188401 - Flags: review?(pinkerton) → review+
(Assignee)

Comment 26

13 years ago
Checked in. Please keep an eye out for this crash to see if we really nailed it
this time. Fix will be in 20050707 and later.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 27

13 years ago
Hmm, I'm seeing bad port assertions when running on Tiger. I see that in our
lockFocus override, where we do:

- (void)lockFocus
{
  SetPort(NULL);
  [super lockFocus];
}

_savePort is being set with an invalid port.
You need to log in before you can comment on or make changes to this bug.