298201 - [10.4] Tiger: Crash at [NSQuickDrawView dealloc]

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect, P1)

Description

[Avi Drissman]

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.

Comment 1

[Avi Drissman]

Oh, yeah. Tiger, 10.4.1.

Comment 2

[Samuel Sidler (old account; do not CC)]

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.

Comment 3

[Avi Drissman]

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?

Comment 4

[Simon Fraser [no longer active]]

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

Comment 5

[Ulrik Sverdrup]

Attachment: 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.

Comment 6

[Mike Pinkerton (not reading bugmail)]

whee i saw this today as well.

Comment 7

[mschulman]

Attachment: 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

[Josh Aas]

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

Comment 9

[Simon Fraser [no longer active]]

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.

Comment 10

[Mike Pinkerton (not reading bugmail)]

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.

Comment 11

[Simon Fraser [no longer active]]

(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

[Joseph Maurer]

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

Comment 13

[Simon Fraser [no longer active]]

(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

[Joseph Maurer]

(In reply to comment #13)

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

Comment 15

[Simon Fraser [no longer active]]

Attachment: 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?

Comment 16

[Mike Pinkerton (not reading bugmail)]

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.

Comment 17

[Josh Aas]

landed on trunk

Comment 18

[Graeme Porteous]

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

Comment 19

[Jeff Williams]

Same here. One of the Talkback IDs is TB7244415G.

Comment 20

[Simon Fraser [no longer active]]

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?

Comment 21

[Joseph Maurer]

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

Comment 22

[Simon Fraser [no longer active]]

Attachment: 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.

Comment 23

[Mike Pinkerton (not reading bugmail)]

+- (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.

Comment 24

[Simon Fraser [no longer active]]

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

Comment 25

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 188401 [details] [diff] [review]
Patch

r=pink

Comment 26

[Simon Fraser [no longer active]]

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.

Comment 27

[Simon Fraser [no longer active]]

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.