616775 - Use OpenNSPRFileDesc instead of OpenANSIFileDesc in nsUpdateDriver.cpp

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

After researching bug 616519 I think OpenNSPRFileDesc should be used instead of OpenANSIFileDesc. Some basic data points:

In the last week crash-stats shows 30 crashes for OpenANSIFileDesc.
Client code that uses OpenANSIFileDesc
/netwerk/protocol/about/nsAboutBloat.cpp (View Hg log or Hg annotations)
    line 143 -- rv = lfile->OpenANSIFileDesc("w", &out);

/js/src/xpconnect/loader/mozJSComponentLoader.cpp (View Hg log or Hg annotations)
    line 1147 -- rv = aComponentFile->OpenANSIFileDesc("r", &fileHandle);

/toolkit/xre/nsUpdateDriver.cpp (View Hg log or Hg annotations)
    line 199 -- rv = statusFile->OpenANSIFileDesc("r", &fp);
    line 217 -- nsresult rv = statusFile->OpenANSIFileDesc("w", &fp);
    line 240 -- rv = versionFile->OpenANSIFileDesc("r", &fp);

/toolkit/profile/src/nsToolkitProfileService.cpp (View Hg log or Hg annotations)
    line 828 -- rv = mListFile->OpenANSIFileDesc("w", &writeFile);


In the last week crash-stats shows 5 crashes for OpenNSPRFileDesc.
Client code that uses OpenNSPRFileDesc
/netwerk/cache/nsDiskCacheBlockFile.cpp (View Hg log or Hg annotations)
    line 61 -- nsresult rv = blockFile->OpenNSPRFileDesc(PR_RDWR | PR_CREATE_FILE, 00600, &mFD);

/netwerk/cache/nsDiskCacheStreams.cpp (View Hg log or Hg annotations)
    line 675 -- rv = mLocalFile->OpenNSPRFileDesc(flags, 00600, fd);

/netwerk/cache/nsDiskCacheMap.cpp (View Hg log or Hg annotations)
    line 81 -- rv = localFile->OpenNSPRFileDesc(PR_RDWR | PR_CREATE_FILE, 00600, &mMapFD);
    line 679 -- rv = file->OpenNSPRFileDesc(PR_RDONLY, 00600, &fd);
    line 829 -- rv = localFile->OpenNSPRFileDesc(PR_RDWR | PR_TRUNCATE | PR_CREATE_FILE, 00600, &fd);

/netwerk/base/src/nsFileStreams.cpp (View Hg log or Hg annotations)
    line 236 -- rv = localFile->OpenNSPRFileDesc(aIOFlags, aPerm, &fd);
    line 577 -- rv = localFile->OpenNSPRFileDesc(ioFlags, perm, &fd);

/netwerk/base/src/nsIncrementalDownload.cpp (View Hg log or Hg annotations)
    line 84 -- nsresult rv = lf->OpenNSPRFileDesc(flags, 0600, &fd);

/parser/htmlparser/tests/logparse/logparse.cpp (View Hg log or Hg annotations)
    line 97 -- localfile->OpenNSPRFileDesc(0660, PR_WRONLY | PR_CREATE_FILE, &outputfile);

/modules/plugin/base/src/nsPluginHost.cpp (View Hg log or Hg annotations)
    line 2471 -- rv = localFile->OpenNSPRFileDesc(PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE, 0600, &fd);
    line 2616 -- rv = localFile->OpenNSPRFileDesc(PR_RDONLY, 0444, &fd);

/modules/libjar/nsZipArchive.cpp (View Hg log or Hg annotations)
    line 184 -- nsresult rv = file->OpenNSPRFileDesc(PR_RDONLY, 0000, &fd);
    line 284 -- rv2 = logFile->OpenNSPRFileDesc(PR_WRONLY|PR_CREATE_FILE|PR_APPEND, 0644, &fd);

/modules/libjar/nsJAR.cpp (View Hg log or Hg annotations)
    line 293 -- rv = localFile->OpenNSPRFileDesc(PR_WRONLY | PR_CREATE_FILE, item->Mode(), &fd);

/xulrunner/app/nsRegisterGREWin.cpp (View Hg log or Hg annotations)
    line 133 -- rv = localSaved->OpenNSPRFileDesc(PR_CREATE_FILE | PR_RDWR, 0664, &fd);
    line 175 -- rv = localSaved->OpenNSPRFileDesc(PR_CREATE_FILE | PR_WRONLY | PR_TRUNCATE, 0664, &fd);
    line 232 -- nsresult rv = localSaved->OpenNSPRFileDesc(PR_RDONLY, 0, &fd);

/js/src/xpconnect/loader/mozJSComponentLoader.cpp (View Hg log or Hg annotations)
    line 1103 -- rv = aComponentFile->OpenNSPRFileDesc(PR_RDONLY, 0, &fileHandle);

/xpcom/io/nsFastLoadFile.cpp (View Hg log or Hg annotations)
    line 929 -- rv = localFile->OpenNSPRFileDesc(PR_RDONLY, 0, &fd);

/xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp (View Hg log or Hg annotations)
    line 136 -- if (NS_FAILED(aFile->OpenNSPRFileDesc(PR_RDONLY, 0444, &fd)) || !fd)

/xpcom/components/nsComponentManager.cpp (View Hg log or Hg annotations)
    line 632 -- rv = aFile->OpenNSPRFileDesc(PR_RDONLY, 0444, &fd);

/toolkit/xre/nsAppRunner.cpp (View Hg log or Hg annotations)
    line 823 -- localFile->OpenNSPRFileDesc(PR_RDWR | PR_APPEND, 0600, &fd);
    line 2439 -- lf->OpenNSPRFileDesc(PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE, 0600, &fd);

/toolkit/xre/nsConsoleWriter.cpp (View Hg log or Hg annotations)
    line 82 -- rv = lfile->OpenNSPRFileDesc(PR_WRONLY | PR_APPEND | PR_CREATE_FILE,

/toolkit/crashreporter/nsExceptionHandler.cpp (View Hg log or Hg annotations)
    line 769 -- nsresult rv = localFile->OpenNSPRFileDesc(PR_WRONLY | PR_CREATE_FILE, 00600,
    line 788 -- nsresult rv = localFile->OpenNSPRFileDesc(PR_RDONLY, 0, &fd);
    line 1540 -- extraFile->OpenNSPRFileDesc(PR_WRONLY | PR_CREATE_FILE | truncOrAppend,

/browser/components/migration/src/nsIEProfileMigrator.cpp (View Hg log or Hg annotations)
    line 1832 -- NS_SUCCEEDED(localCookieFile->OpenNSPRFileDesc(PR_RDONLY, 0444, &fd))) {

/content/media/nsMediaCache.cpp (View Hg log or Hg annotations)
    line 591 -- rv = tmpFile->OpenNSPRFileDesc(PR_RDWR | nsILocalFile::DELETE_ON_CLOSE,

/security/manager/ssl/src/nsPKCS12Blob.cpp (View Hg log or Hg annotations)
    line 473 -- rv = file->OpenNSPRFileDesc(PR_RDWR|PR_CREATE_FILE|PR_TRUNCATE, 0664,

/security/manager/ssl/src/nsNSSCertificateDB.cpp (View Hg log or Hg annotations)
    line 1126 -- rv = aFile->OpenNSPRFileDesc(PR_RDONLY, 0, &fd);

/profile/dirserviceprovider/src/nsProfileLock.cpp (View Hg log or Hg annotations)
    line 514 -- rv = lockFile->OpenNSPRFileDesc(PR_RDONLY, 0, &fd);

Considering that OpenNSPRFileDesc has only 1 crash for every 6 OpenANSIFileDesc crashes over a period of one week while OpenNSPRFileDesc is used much more extensively I think it would be a good thing if OpenNSPRFileDesc was used instead of OpenANSIFileDesc in nsUpdateDriver.cpp.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 2

[Justin Dolske [:Dolske]]

(In reply to comment #0)

> Considering that OpenNSPRFileDesc has only 1 crash for every 6 OpenANSIFileDesc
> crashes over a period of one week while OpenNSPRFileDesc is used much more
> extensively

I don't really find that logic compelling; could just be that there's a single OpenANSIFileDesc() caller that's either in a hot path, or called where there's already memory corruption for some reason. If it's our code we should figure out why it's crashy, if it's OS code we should deprecate the API.

That said, OpenANSIFileDesc seems to be using stdio (fopen), which at least on 32-bit Solaris limited you to 256 open files (I think other platforms are OK these days?). So happy to see it go anyway. :)

Comment 3

[Robert Strong (they/them - no direct email)]

(In reply to comment #2)
> (In reply to comment #0)
> 
> > Considering that OpenNSPRFileDesc has only 1 crash for every 6 OpenANSIFileDesc
> > crashes over a period of one week while OpenNSPRFileDesc is used much more
> > extensively
> 
> I don't really find that logic compelling; could just be that there's a single
> OpenANSIFileDesc() caller that's either in a hot path, or called where there's
> already memory corruption for some reason. If it's our code we should figure
> out why it's crashy, if it's OS code we should deprecate the API.
That would be bug 616519.

I'm taking the stance that app update is a special case and I am unwilling to wait to figure out why OpenANSIFileDesc is less stable especially when it has been happening for quite some time. As far as I'm concerned, when possible a stabler API should always be used immediately whenever an API being used by app update exhibits even a hint of not being stable.

Comment 4

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/dbb99009d49e

This code path is tested by test_0200_app_launch_apply_update.js

Comment 5

[Robert Strong (they/them - no direct email)]

Attachment: patch as pushed

Discussed the perms that should be used with Mossop and decided upon 0660