377203 - crash [@ libobjc.A.dylib][@ CHBrowserListener::OnLocationChange], often at startup

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: General, defect)

Description

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

Attachment: Talkback report

A large number of our recent libobjc crashes in Talkback have this sort of stack.  Mostly at/around startup (based on "Since Last Crash"), but a few with some larger SLC times.

There's only one with any "useful" comment:

TB30162322x - "starting new nightly build with previously opened windows and GrowlCamino 1.0 installed"

TB30116804x
TB30481143x -- not at startup
TB30161823x
TB30163419x
TB30217029x
TB30237461x
TB30237576x
TB30655789x
TB30501215x -- not at startup
TB30556065x -- not at startup
TB30729718x -- not at startup

Comment 1

[Stuart Morgan]

My gut feeling from the stacks is that GrowlCamino is not involved; these all appear to be during the creation of an empty window, which doesn't sound like something that would go through GrowlCamino code paths.

Comment 2

[froodian (Ian Leue)]

Attachment: Patch?

This is kind of a shot in the dark, but I do know that |stringWithUTF8String| causes a crash if it gets sent NULL, and it seems like a good idea to make sure we're actually put something in spec regardless.

Comment 3

[Stuart Morgan]

Comment on attachment 261337 [details] [diff] [review]
Patch?

That line bothered me when I looked at this as well, but since it's not actually possible for GetSpec to fail as far as I can tell, it's really unlikely to be the issue (plus I would expect to see stringWithUTF8String: on the stack).

I'm fine with bullet-proofing it anyway, but I think the right way to do it is to check for spec.get() being NULL rather that checking the return value from GetSpec, since that's what we are actually trying to prevent.

Comment 4

[froodian (Ian Leue)]

Attachment: Bullet-proofing, v2

Comment 5

[Stuart Morgan]

Comment on attachment 261602 [details] [diff] [review]
Bullet-proofing, v2

>+  if (strbuff == NULL)

if (!strbuff)

r=me with that change.

Comment 6

[froodian (Ian Leue)]

Attachment: Bullet-proofing, v2.1

Comment 7

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 261603 [details] [diff] [review]
Bullet-proofing, v2.1

sr=pink, what's the result of returning error_failure here when we weren't before? will that someties cause us to not change the url bar because gecko terminates a page load early, etc etc?

Comment 8

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

Ian, was this ever checked in, or had pink's questions never been resolved?

Comment 9

[Stuart Morgan]

We should watch 1.6 talkback, and see if this is still showing up.

Comment 10

[Chris Lawson (gone)]

(In reply to comment #8)
> Ian, was this ever checked in, or had pink's questions never been resolved?

It was never checked in. I don't know if anyone ever answered pink's question or not, but I'm guessing not.

(In reply to comment #9)
> We should watch 1.6 talkback, and see if this is still showing up.

Smokey did a search (and I'm still in the process of doing one myself) and found nothing that looks like this bug in either 1.6 or 2.0.

Do we still want to do anything about this?

Comment 11

[Wayne Mery (:wsmwk)]

(In reply to comment #10)
> (In reply to comment #8)
> > Ian, was this ever checked in, or had pink's questions never been resolved?
> 
> It was never checked in. I don't know if anyone ever answered pink's question
> or not, but I'm guessing not.
> 
> (In reply to comment #9)
> > We should watch 1.6 talkback, and see if this is still showing up.
> 
> Smokey did a search (and I'm still in the process of doing one myself) and
> found nothing that looks like this bug in either 1.6 or 2.0.
> 
> Do we still want to do anything about this?

and there is nothing on crash-stats

WFM?

Comment 12

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

Stuart, is it worth checking the equivalent of this patch in, or just WFMing this bug?  There's nothing related to CHBL::OnLocationChange in Breakpad for the last 4 weeks (and the only CHBL-related crash is your simulated crash from bug 512730).

Note, fwiw, that bug 441733 swapped the order of a bunch of pieces of this function (see http://hg.mozilla.org/camino/diff/b50671eb0e00/src/embedding/CHBrowserListener.mm), so that we get the spec and make an NSString from it before checking aRequest and the status.

Comment 13

[Stuart Morgan]

We should figure out if returning failure is really the right thing before we land anything.