322374 - Crash [@ libobjc.A.dylib.227.0.0] when clearing download list

Status: VERIFIED FIXED

(Camino Graveyard :: Downloading, defect)

Description

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

We're quite a few Talkback reports of a crash when clearing the download manager.

See tb13486349

Comment 1

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

This is one of our topcrashers and should block 1.0.

Comment 2

[Nick Kreeger]

Attachment: Proposed patch

Patch just makes us careful when we call to mProgressController in ProgressView.

Comment 3

[Nick Kreeger]

Attachment: Updated Patch

Same patch as before for ProgressView, but move [self viewDidLoad] out of |init| and to |awakeFromNib| in ProgressViewController. This should help rid ProgressView of any nil values, but lets keep the nil checking in just in case.

Comment 4

[Mark Mentovai]

trunk, branch, and other branch.

Comment 5

[Stuart Morgan]

Not that it really matters, but since this doesn't return anything:

 -(void)setSelected:(BOOL)inSelected
 {
+  // make sure the controller is not nil before setting its selected state
+  if (!mProgressController)
+    return;
+  
   [mProgressController setSelected:inSelected];
 }

the nil check isn't doing anything.

Comment 6

[Simon Fraser [no longer active]]

Comment on attachment 207519 [details] [diff] [review]
Updated Patch

>Index: download/ProgressView.mm
>===================================================================

>   if (self) {
>     mLastModifier = kNoKey;
>+    mProgressController = nil;

Obj-C inits member vars to 0 anyway, so this is a no-op.

> - (BOOL)isSelected
> {
>+  // make sure the controller is not nil before checking if it is selected
>+  if (!mProgressController)
>+    return NO;
>+  
>   return [mProgressController isSelected];

No effect here either; calling methods on a nil object is legal, and will return a zero value (hence NO).


> -(void)setSelected:(BOOL)inSelected
> {
>+  // make sure the controller is not nil before setting its selected state
>+  if (!mProgressController)
>+    return;
>+  
>   [mProgressController setSelected:inSelected];

Also a no-op change. 

>Index: download/ProgressViewController.mm
>===================================================================

> -(id)init
> {
>   if ((self = [super init]))
>   {
>     [NSBundle loadNibNamed:@"ProgressView" owner:self];
>-    [self viewDidLoad];
>   }
>   return self;
> }
> 
>+-(void)awakeFromNib
>+{
>+  [self viewDidLoad];
>+}
>+
> -(void)dealloc

I don't see what difference this makes either.

Comment 7

[Simon Fraser [no longer active]]

Reopening, becuase I see nothing in the patch that will fix this. Did anyone actually reproduce the crash?

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I just tried to repro a couple of time and couldn't, once with several days worth of accumulated dls and one active, and once with one accumulated dl and two active.

And then I noticed the talkback reports are all from 10.4.3 machines.... :-(

Comment 9

[Stuart Morgan]

> No effect here either; calling methods on a nil object is legal, and 
> will return a zero value (hence NO).

While there's no effect in the BOOL case, that change should stay--it's only really defined explicitly to behave that way for id types, so relying on it for anything else is bad style.

Comment 10

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

I've tried to reproduce a few times, but can't (using 10.4.3). People *are* seeing this though. And it looks like they saw it in 1.0b1 as well.

Comment 11

[Simon Fraser [no longer active]]

(In reply to comment #9)
> > No effect here either; calling methods on a nil object is legal, and 
> > will return a zero value (hence NO).
> 
> While there's no effect in the BOOL case, that change should stay--it's only
> really defined explicitly to behave that way for id types, so relying on it for
> anything else is bad style.

http://ridiculousfish.com/blog/?p=14
It basically leaves 0 in the register, so any type that fits in a register and where "zero" is all 0 bytes is fine (BOOL, int, pointers, id etc). float is bad (esp. on intel), structs are bad.

Comment 12

[Stuart Morgan]

(In reply to comment #11)
> It basically leaves 0 in the register, so any type that fits in a register and
> where "zero" is all 0 bytes is fine (BOOL, int, pointers, id etc).

Right, it works, but is "considered bad style" (http://developer.apple.com/documentation/Cocoa/Conceptual/ObjectiveC/LanguageOverview/chapter_4_section_3.html) for non-id return because it only works as a side-effect of the explicitly defined behavior that it works for ids.

Comment 13

[Stuart Morgan]

Isn't there a good chance that the real problem here is that PVC calls

  [mCompletedView setController:self];
  [mProgressView setController:self];

and doesn't setController:nil on both in it's dealloc?

Comment 14

[Nick Kreeger]

I tried several different scenarios to attempt to reproduce this crash. I was never able to sucessfully recreate the crash with 1.0 b2. But I was able to create the Camino to lock up and beachball for a good minute when I had several downloads in the list and one current download that was running, right as the current download ended, I pressed clean up, thus causing the hang. I have been trying to run down the glitch this past weekend with no luck. My only assumption is that right as the download ends, there is a collision with the file system subscriptions and other old subscriptions that maybe attempting to un-subscribe.

Looking at the talkback reports, it doesn't seem to matter how may downloads there are to cause the crash.

I will continue to try to recreate this crash, and track down the possible problem.

Comment 15

[Nick Kreeger]

Attachment: Patch using Stuart's suggestion

This is a patch from Stuart's suggetion, can't hurt to try it?

Comment 16

[Simon Fraser [no longer active]]

I checked in this patch. Let's see if the crash goes away.

Comment 17

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

We're not seeing this crash anymore, afaict.

Verifying as fixed on 1.8.0 branch and trunk (can't verify on 1.8.1 since there are no builds off the 1.8 branch).

Comment 18

[Nick Kreeger]

Reopening since we are seeing this again.

Comment 19

[Stuart Morgan]

(In reply to comment #18)
> Reopening since we are seeing this again.

Could you post some Talkback links and/or crash logs?  I didn't see anything in Talkback.

Comment 20

[Adam Guthrie]

It's currently the #1 topcrash for Camino 1.1.x according to http://talkback-public.mozilla.org/reports/camino/CM11x.

Incident ID: 22898145
Stack Signature	libobjc.A.dylib.227.0.0 + 0x52e8 (0x90a3e2e8) b376cdd4
Product ID	Camino11
Build ID	2006090201
Trigger Time	2006-09-04 16:09:50.0
Platform	MacOSX
Operating System	Darwin 8.7.0
Module	libobjc.A.dylib.227.0.0 + (000052e8)
URL visited	
User Comments	
Since Last Crash	8843 sec
Total Uptime	8843 sec
Trigger Reason	SIGSEGV: Segmentation Violation: (signal 11)
Source File, Line No.	N/A
Stack Trace 	
libobjc.A.dylib.227.0.0 + 0x52e8 (0x90a3e2e8)
libobjc.A.dylib.227.0.0 + 0x5280 (0x90a3e280)
nsDownloadListener::OnStateChange()  [mozilla/extensions/transformiix/source/xslt/txXSLTNumberCounters.cpp]
nsWebBrowserPersist::OnStopRequest()  [mozilla/extensions/transformiix/source/xslt/txXSLTNumberCounters.cp]
nsHttpChannel::OnStopRequest()  [mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp, line 713]
nsInputStreamPump::OnStateStop()  [mozilla/extensions/transformiix/source/xslt/txXSLTNumberCounters.cpp, l]
nsInputStreamPump::OnInputStreamReady()  [mozilla/extensions/transformiix/source/xslt/txXSLTNumberCounters]
nsInputStreamReadyEvent::EventHandler()
PL_HandleEvent()
PL_ProcessPendingEvents()
__CFRunLoopDoSources0()
__CFRunLoopRun()
CFRunLoopRunSpecific()
RunCurrentEventLoopInMode()
ReceiveNextEventCommon()
BlockUntilNextEventMatchingListInMode()
_DPSNextEvent()
-   -   NSApplicationMain()
_start()   start()

Comment 21

[Stuart Morgan]

From what I could tell, "Things ending in libobjc.A.dylib.227.0.0" is the topcrasher. I'm was looking for evidence that it has to do with clearing downloads (and thus is related at all to this bug).

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Yeah, the comments (where there are any) are all indicating actual downloading going on, rather than clearing the downloads list (as this bug was originally opened against).  I've hit a random collection of other reports from the 161 and found some that do share the stack with the downloading ones (as well as the usual completely random libobjc.A.dylib collection).

I suspect these are two different bugs, but putting this on the 1.1 list so as not to lose it in the event they aren't.

Comment 23

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

I see no indication that this is the same bug as reported before. The only TB report that lists clearing a download list is TB22176964. All the rest have different (or no) comments. I recommend re-closing this bug and filing a new one for the issues seen in the comments of those Talkback reports.

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Setting this back to FIXED per irc.  The new talkbacks (the ones remotely about downloading) are different.

Comment 25

[Adam Guthrie]

I filed bug 351504 for the 1.1.x version of this crash.