299090 - Crash if addSearchEngine called with invalid search plugin file but valid search icon (mycroft search engine) [@ nsCharTraits::length]

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Laurence "GreenReaper" Parry]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050427 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050427 Firefox/1.0+

It is possible to fool firefox into adding a search engine that does not exist
if you specify a valid icon for that search engine in the call to
addSearchEngine(). Selecting this search engine will then crash Firefox.

Confusingly, it will not do this the next time you try it, because it will not
add the search engine if an icon file with the specified name already exists,
and it already downloaded it into the searchplugins folder.

Crucially it does not seem to check for the existence of the src file for the
plugin before adding it to the list.

Reproducible: Always

Steps to Reproduce:
Make a call to window.sidebar.addSearchEngine() within the following restrictions:
1. The search plugin icon must be a valid file
2. The search plugin icon file must not already be downloaded (either because
it's used by an existing plugin, or because you've tried this before)
3. The search plugin source file must *not* be a valid file (or not accessible)

Then,
4. Select the search entry (which will have an icon but no text) in the search
engine dropdown.
Actual Results:  
Firefox downloaded the icon but not the source file (which didn't exist) without
complaint. It added an item to the end of the list of search engines with the
icon specified, but no text. When this item was selected, Firefox crashed.

Expected Results:  
If the file was not found, the icon and entry in the list should not have been
added (this is what Mozilla does). Some sort of error message would have been
nice, too. :-)

Attaching testcase (as linked) and stack trace.

Comment 1

[Laurence "GreenReaper" Parry]

Attachment: Testcase showing the problem

Comment 2

[Laurence "GreenReaper" Parry]

Attachment: Stack trace of crash on click

Comment 3

[timeless]

Comment on attachment 187598 [details]
Stack trace of crash on click

this doesn't look right, what kind of build did you use and what debugger?

Comment 4

[Laurence "GreenReaper" Parry]

(In reply to comment #3)
> (From update of attachment 187598 [details] [edit])
> this doesn't look right, what kind of build did you use and what debugger?
> 

Microsoft Visual Studio 2003. I don't have symbols or source for Firefox, it's
just the call stack for the thread that was shown as highlighted. This is from
debugging when it throws up the error.

I was using it with Bluefyre builds, I guess that messed up the stack trace.
Let's try an hourly build from a few hours ago . . .

Comment 5

[Laurence "GreenReaper" Parry]

Attachment: Stack trace, version 2

Comment 6

[Laurence "GreenReaper" Parry]

I'm guessing that's not going to be much use either - either way, they're
optimized bulids. It should be easy to duplicate this crash with the testcase,
though.

Comment 7

[timeless]

Comment on attachment 187607 [details]
Stack trace, version 2

why not install talkback and let it report the crash? without symbols, all you
have are exports and offsets from them, which are rarely helpful.

Comment 8

[Laurence "GreenReaper" Parry]

(In reply to comment #7)
> (From update of attachment 187607 [details] [edit])
> why not install talkback and let it report the crash? without symbols, all you
> have are exports and offsets from them, which are rarely helpful.
> 

Well, the main reason would be that whenever I try to install a build having
selected the TalkBack feature, it falls over while it's doing that part of the
install:

---------------------------
Error
---------------------------
Error occurred during installation - Quality Feedback Agent: -214 DOES_NOT_EXIST
---------------------------
OK   
---------------------------

from install_wizard.log

Quality Feedback Utility (version 1.0.5)
------------------------

** initInstall: 0
** communicatorFolder: C:\Program Files\Communications\Mozilla Firefox\
** File to delete: C:\Program Files\Communications\Mozilla
Firefox\components\l10n.ini\
** addDirectory() returned: -214

Install **FAILED** with error -214  --  2005-06-29 08:17:44

Comment 9

[Jaime Mitchell (use [e-mail] for email)]

> > why not install talkback and let it report the crash? without symbols, all you

> Well, the main reason would be that whenever I try to install a build having
> selected the TalkBack feature, it falls over while it's doing that part of the
> install:

The build ID you are using is very old. Try with this build
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-1.0+.en-US.win32.installer.exe
and selecting Mozilla Quality Feedback Agent during a custom install.

Comment 10

[Andrew Schultz]

Attachment: stacktrace

nsPrefLocalizedString::SetData is getting a NULL pointer

Comment 11

[Andrew Schultz]

confirmed with current firefox CVS

Comment 12

[timeless]

ok, the crash will be handled elsewhere.

i'm betting the js cause is
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/search.xml&rev=1.23&mark=182#168

but i'm waiting for confirmation from someone. once the crash is fixed, someone
needs to figure out if the js in firefox is behaving reasonably, and if not, fix it.

Comment 13

[O. Atsushi (Torisugari)]

(In reply to comment #12)
> ok, the crash will be handled elsewhere.

Is there any chance bug 299146 will be fixed on 1.0 branch as well?

According to bug 271287#c2 , more than 10 guys/day (firefox 1.0 - 1.0.6 users)
crash their browser due to this bug.

> i'm betting the js cause is 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/search.xml&rev=1.23&mark=182#168

It seems to me you are right.

> but i'm waiting for confirmation from someone. once the crash is fixed,
> someone needs to figure out if the js in firefox is behaving reasonably,
> and if not, fix it.

Empty string would be handled correctly after restart the browser (the default
engine will be selected). So it's not that bad to set the pref null.

Maybe we should not show such a crappy engine in the list, in the first place.

Comment 14

[Adam Guthrie]

This was fixed by the checkin for bug 299146. Follow-up bugs should be filed separately.