428456 - no strings for some onlineTooltipN

Status: NEW

(SeaMonkey :: UI Design, defect)

Description

[Andrew Schultz]

bug 223908 added different tooltips for different values of the network.proxy.type.  Mine is set to 5 and I get no tooltip.  This results in

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://communicator/content/utilityOverlay.js :: setOfflineUI :: line 220"  data: no]

this was suppressed until today (bug 415498, apparently)

Comment 1

[Robert Kaiser]

The fault seems to be that http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/suite/common/utilityOverlay.js&rev=1.111&mark=219-220#202 just retrieves a value from a pref (or not if it throws) and appends it to a string to be used as ID for retrieving the tooltip without checking for valid values.

Comment 2

[[e-mail]]

What do you mean by a valid value? 5 isn't currently implemented in the Preferences dialog yet.

Comment 3

[Robert Kaiser]

valid string ID values is what I mean. Of course, it's not the job of this code to say what pref values are valid, but it _is_ its job to only call valid string IDs.

Comment 4

[Andrew Schultz]

> What do you mean by a valid value? 5 isn't currently implemented in the
> Preferences dialog yet.

That doesn't stop it from being the default value:
http://mxr.mozilla.org/seamonkey/source/modules/libpref/src/init/all.js#791

Practically speaking, I have no idea what 5 means...

Comment 5

[Robert Kaiser]

Apparently, it's "Use system proxy settings", see bug 416274 - and a value 4 exists as well, according to their pref panel.

Comment 6

[[e-mail]]

Actually we have a string for 4. (3 is obsolete, and means 0.)
For some reason, illegal values default to 1 (manual proxy settings).

Anyway, I guess we need a bug on adding UI for this new value.

Comment 7

[Robert Kaiser]

Seems like that. And in this bug, we should at least catch the exception and ideally not even try to get a string when getting the pref failed and the var is not set.

Comment 8

[[e-mail]]

I don't think getting the pref is likely to fail ;-)

Comment 9

[Robert Kaiser]

(In reply to comment #8)
> I don't think getting the pref is likely to fail ;-)

In that case, we should remove the try/catch around it. Trying to add an undefined var to what we hand to .GetStringFromName() does not sound like a good idea in any case to me, and the code reads in a way that this is possible.

Comment 10

[[e-mail]]

(In reply to comment #9)
> (In reply to comment #8)
> > I don't think getting the pref is likely to fail ;-)
> In that case, we should remove the try/catch around it. Trying to add an
> undefined var to what we hand to .GetStringFromName() does not sound like a
> good idea in any case to me, and the code reads in a way that this is possible.
Hmm, I wonder who reviewed that :-[

Comment 11

[Robert Kaiser]

The actual error was fixed in an additional patch for bug 416274 - do we still want to fix the try/catch and/or make it safer against wrong/future pref values?

Comment 12

[Martin Mokrejs]

It is not clear to me whether the fix is ought to be applied over TRUNK but it
seems not. I am getting as of now the following:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE)
[nsIStringBundle.GetStringFromName]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"
 location: "JS frame :: chrome://communicator/content/utilityOverlay.js ::
setOfflineUI :: line 220"  data: no]
************************************************************


Build platform
target
i686-pc-linux-gnu

Build tools
Compiler        Version         Compiler flags
gcc     gcc version 4.3.1 (Gentoo 4.3.1-r1 p1.1)        -Wall -W -Wno-unused
-Wpointer-arith -Wcast-align -W -Wno-long-long -pedantic -fno-strict-aliasing
-pthread -pipe
c++     gcc version 4.3.1 (Gentoo 4.3.1-r1 p1.1)        -fno-rtti
-fno-exceptions -Wall -Wconversion -Wpointer-arith -Woverloaded-virtual -Wsynth
-Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-long-long
-pedantic -fno-strict-aliasing -fshort-wchar -pthread -pipe

Configure arguments
--disable-optimize '--enable-debug=-g3\ -O0\ -ggdb' --enable-debug-modules=all
--enable-debugger-info-modules --enable-detect-webshell-leaks --enable-svg
--enable-svg-renderer-libart --enable-image-decoders=all --with-qtdir=/usr/qt/3
--enable-application=suite --disable-freetype2 --enable-jprof
--enable-default-toolkit=cairo-gtk2 --enable-xft --disable-gssapi

Comment 13

[[e-mail]]

What do you mean by trunk? We switched from CVS to Hg before the fix landed.

Comment 14

[Martin Mokrejs]

Whatever I have fetched from the old CVS then. :((((( Why isn't it shutdown or having a different password?

Comment 15

[Robert Kaiser]

(In reply to comment #14)
> Whatever I have fetched from the old CVS then. :((((( Why isn't it shutdown or
> having a different password?

Neither cvs nor hg have passwords, we're OPEN source, you know. :)

That said, maybe we should display some kind of warnings when people try to build SeaMonkey or Thunderbird from cvs - but that's something for other bugs.

I still wonder about my questions in comment #11 though.