668725 - nsMIMEInfoOS2::LoadUriInternal(), nsOSHelperAppService::OSProtocolHandlerExists() and nsOSHelperAppService::GetApplicationDescription() access wrong pref

Status: RESOLVED WONTFIX

(Core Graveyard :: File Handling, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

nsMIMEInfoOS2::LoadUriInternal(), nsOSHelperAppService::OSProtocolHandlerExists() and nsOSHelperAppService::GetApplicationDescription() are now have following code.

nsCAutoString prefName = branchName + branchName;
nsAdoptingCString prefString = Preferences::GetCString(prefName.get());

The pref name is very strange. Old code (pre bug 666903) got the branch of the branchName and passes the branchName to nsIPrefBranch::GetCharPref() again. Therefore, the branch accessed branchName + branchName.

Comment 2

[Walter Meinl]

Attachment: just a reminder that this line has to be added to a right solution -- not yet for review

This unbreaks the build w/o correcting yet the code that is in doubt.

Comment 3

[Walter Meinl]

(In reply to Masayuki Nakano (Mozilla Japan) from comment #0)
> nsMIMEInfoOS2::LoadUriInternal(),
> nsOSHelperAppService::OSProtocolHandlerExists() and
> nsOSHelperAppService::GetApplicationDescription() are now have following
> code.
> 
> nsCAutoString prefName = branchName + branchName;
> nsAdoptingCString prefString = Preferences::GetCString(prefName.get());
> 
> The pref name is very strange. Old code (pre bug 666903) got the branch of
> the branchName and passes the branchName to nsIPrefBranch::GetCharPref()
> again. Therefore, the branch accessed branchName + branchName.

The code in question was introduced in bug390075
here's the checkin log
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/uriloader/exthandler/os2&command=DIFF_FRAMESET&file=nsMIMEInfoOS2.cpp&rev2=1.12&rev1=1.11

Peter, since you introduced the code, you may have an idea (when looking at the link above) why you solved the problem of bug390075 the way you did.

Comment 4

[Peter Weilbacher]

I'm pretty sure that my cde did not do the equivalent of branchName + branchName. AFAICT it got the pref branch, then used GetCharPref() on it to access the content of the pref. It could have been wrong or not very elegant, but nobody ever complained that it did not work...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yes, but it included the path to the pref in both the branch it got and the pref name it used.  From http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp&rev=1.12&root=/cvsroot :

180 rv = thePrefsService->GetBranch(prefName.get(), getter_AddRefs(prefBranch));
...
183 rv = prefBranch->GetCharPref(prefName.get(), getter_Copies(prefString));

That is _exactly_ concatenating prefName with itself.

I can totally believe that no one tried using it, hence never complained it didn't work...

Comment 6

[Peter Weilbacher]

Well, surely _you_ never used it...

Anyway, apparently I and two other people were wrong in 2007, so someone please fix so that it does whatever it was meant to do (which was apparently the "applications.epm" trick as listed on <http://www.os2bbs.com/os2news/Warpzilla.html>).

Comment 7

[Mats Palmgren (inactive)]

Sorry, but OS/2 support has been removed from mozilla-central (bug 969757).