67900 - Tools->History crashes

Status: VERIFIED FIXED

(Core Graveyard :: RDF, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

FreeBSD 4.1. 20010206xx

Pull/build today.  Also crashed on 1/31 build.

Select Tools->History.  Boom.  GDB trace to follow.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: GDB backtrace (2001-1-31 source; crashes with 2/6 as well)

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Crash, critical, mozilla0.8 (excuse me for giving it a milestone, it seemed
important to get this fixed for 0.8 and time is short), OS->All (from the error
it would appear to affect all OS's; not checked)

I have a work-around patch to fix this.  There's a call in nsXULSortService.cpp:

	gXULUtils->GetElementResource(child, &resource);

This call can fail, and now does.  It then tries to use resource and Boom. 
Workaround:  check the result.  (Patch to be attached)  This doesn't solve the
fundamental problem of _why_ it fails.  And the patch is VERY crude; I don't
understand the code here well enough to know if there's anything else that
should be done.

Changing to RDF; reassigning to RDF owner.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dumb patch to prevent crash

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: gdb backtrace for two runs w/ 20010207xx pull

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Chris - I also noted there seem to be serious weirdness going on in the History
window as I paged down - columns moving back and forth, etc, incomplete updates,
etc.  I'm rebuilding a non-optimized build and will recheck.  Also, my patch
might be screwing things up.

Comment 6

[Brendan Eich [:brendan]]

Randal, you can always use the milestone name as a keyword to nominate a bug,
and leave the Target Milestone setting to the bug's assignee.  Not that waterson
minded, in this case, but it's better protocol in general.

/be

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Sorry, mea culpa - the fact that 0.8 is coming so soon and a menu entry is
crashing had me worried.

Comment 8

[Chris Waterson]

rjesup: could you try this patch instead? I think it might be allright for 
GetElementResource() to fail here, leaving the nsIRDFResource null. (I cannot 
reproduce the crash, FWIW...)

Comment 9

[Chris Waterson]

Attachment: change bare nsIRDFResource to nsCOMPtr, which will be initialized to 0

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

I'll give it a try.  I got the crash because the stack-garbage value I got was
usually 0x0a.  If a NULL resource is valid; initializing it to NULL would work
as well.

Ok, fixed with that patch.  Note that a bunch of other problems with RDF and
History still exist; I'll open another bug or 3.

Comment 11

[Chris Waterson]

rjesup, can I treat that as an `r='? alecf, could you sr=? cc'ing putterman, who 
wrote this code originally (or, touched it last, anyway) so he gets a chance to 
comment...

Comment 12

[scottputterman]

looks ok to me.  I assume there were no AddRefs or Releases in parts of the code
not in the diff.

Comment 13

[Alec Flett]

fortunately, it wouldn't compile if there were other addref/releases
so sr=alecf! Let's get this puppy in. 

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Yes, that was r=jesup.  (Hmm, didn't know I could 'r').

Comment 15

[Chris Waterson]

Fix checked in. Good find, rjesup!

Comment 16

[Brendan Eich [:brendan]]

Anyone can r=, although mozilla.org wants a module owner or peer (acting for
owner) to do so, in general.  Extra r='s don't hurt, and may help (you're a peer
if you know the code well enough that people believe your comments).

/be

Comment 17

[Tom Everingham]

verified