430058 - Trying to add certain search plugins from Mycroft causes a crash

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Stuart Morgan]

As reported in bug 410958 comment 6:
1) Go to
http://mycroft.mozdev.org/download.html?name=allmusic&sherlock=&opensearch=yes&submitform=Search
2) Click allmusic - Artist/Group
3) Boom (and not the good Steve Jobs kind)

Comment 1

[Mycroft Project (CC)]

Oops...

Is there a good way of identifying Camino so that I can try and save your users from hitting this?

something similar to the IE7 test:
if ((typeof window.external.AddSearchProvider == "unknown") && meth == "p") {
  alert("This plugin uses POST...");

Comment 2

[Sean Murphy]

Darn it!  Found the problem at least...

The crash occurs in the code added to handle <Param> elements.  In cases where a search engine url is not supported, the param handling code attempts to create a NSMutableString from the base search URL, assuming it exists when it is actually nil.  I chose instead to check the return value from -[NSMutableString stringWithString:] thinking for some reason that nil arguments were permitted and nil value just returned back; That's obviously not the case, and the nil argument warning is documented.

I'll get a patch in right away.

Comment 3

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

Yeah, we definitely need to fix this in 1.6.1.

Sean, are there other similar cases in that area of code that could cause a crash? 

Comment 4

[Stuart Morgan]

Yes, there's also the case where template may not exist, so the condition before trying to interact with it should be changed to:

    if ([self browserSupportsSearchQueryURLWithMIMEType:mimeType requestMethod:method] &&
        [attributeDict objectForKey:@"template"])

Comment 5

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

Charles, unfortunately the most reliable way for you to exclude us from POST plug-ins until we fix this (and/or bug 430070 and/or bug 430067) is to look for navigator.vendor = Camino

Comment 6

[Sean Murphy]

Attachment: Fix

Sorry for letting this one get in guys.

Thanks for making us aware of the issue Charles, and I also apologize for the resulting inconvience on your site.

On a side note, the crash occurs because of Gecko's vulnerability to Objective-C expressions.  They can really cause a mess if allowed to trickle down into Gecko frames, so Core has no choice but to capture them and force a crash by dereferencing a NULL pointer: <http://mxr.mozilla.org/mozilla/source/xpcom/base/nsObjCExceptions.h#57>.

Comment 7

[Stuart Morgan]

(In reply to comment #6)
> On a side note, the crash occurs because of Gecko's vulnerability to
> Objective-C expressions.

Yep, thus bug 430071, which should have been part of my original code. Branch doesn't actually do the catch-and-crash in core; the reason it crashes in 1.6 is that the exception unwinds the whole c++ stack uncleanly and leaves things in a really bad state. The trunk changes make the crash immediate and obvious.

Comment 8

[Stuart Morgan]

Comment on attachment 316834 [details] [diff] [review]
Fix

>-    NSMutableString *searchURL = [NSMutableString stringWithString:[self searchEngineURL]];
>-    if (!searchURL)
>+    NSString *baseSearchURL = [self searchEngineURL];
>+    if (!baseSearchURL)
>       return;

Rather than having the unneeded local variable, how about just
  NSMutableString *searchURL = [[[self searchEngineURL] mutableCopy] autorelease];

Comment 9

[Sean Murphy]

Attachment: Fix

(In reply to comment #8)

Yes, much cleaner; Thanks.

Comment 10

[Stuart Morgan]

Comment on attachment 316853 [details] [diff] [review]
Fix

r/sr=smorgan.

Comment 11

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

Checked in on the trunk and the MOZILLA_1_8_BRANCH for 1.6.1.