Closed Bug 322374 Opened 16 years ago Closed 15 years ago

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

Categories

(Camino Graveyard :: Downloading, defect)

1.8 Branch
PowerPC
macOS
defect
Not set
critical

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: samuel.sidler+old, Assigned: nick.kreeger)

References

()

Details

(4 keywords)

Crash Data

Attachments

(2 files, 1 obsolete file)

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

See tb13486349
This is one of our topcrashers and should block 1.0.
Flags: camino1.0+
Attached patch Proposed patch (obsolete) — Splinter Review
Patch just makes us careful when we call to mProgressController in ProgressView.
Assignee: mikepinkerton → nick.kreeger
Status: NEW → ASSIGNED
Keywords: crash
Attached patch Updated PatchSplinter Review
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.
Attachment #207515 - Attachment is obsolete: true
Attachment #207519 - Flags: review+
trunk, branch, and other branch.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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 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.
Reopening, becuase I see nothing in the patch that will fix this. Did anyone actually reproduce the crash?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.... :-(
> 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.
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.
(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.
(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.
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?
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.
This is a patch from Stuart's suggetion, can't hurt to try it?
I checked in this patch. Let's see if the crash goes away.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
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).
Status: RESOLVED → VERIFIED
Reopening since we are seeing this again.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(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.
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()
Severity: normal → critical
OS: Mac OS X 10.2 → Mac OS X 10.4
Target Milestone: Camino1.0 → ---
Version: Trunk → 1.8 Branch
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).
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.
Target Milestone: --- → Camino1.1
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.
Setting this back to FIXED per irc.  The new talkbacks (the ones remotely about downloading) are different.
Status: REOPENED → RESOLVED
Closed: 16 years ago15 years ago
Resolution: --- → FIXED
Target Milestone: Camino1.1 → ---
I filed bug 351504 for the 1.1.x version of this crash. 
Status: RESOLVED → VERIFIED
Crash Signature: [@ libobjc.A.dylib.227.0.0]
You need to log in before you can comment on or make changes to this bug.