Use OpenNSPRFileDesc instead of OpenANSIFileDesc in nsUpdateDriver.cpp

RESOLVED FIXED in mozilla2.0b8

Status

()

Toolkit
Application Update
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

Trunk
mozilla2.0b8
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Attachment #495348 - Attachment description: patch in progress → patch rev1
Attachment #495348 - Flags: review?(dtownsend)
Attachment #495348 - Flags: approval2.0?
Attachment #495348 - Flags: review?(dtownsend)
Attachment #495348 - Flags: review+
Attachment #495348 - Flags: approval2.0?
Attachment #495348 - Flags: approval2.0+
(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. :)
(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.
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Created attachment 495915 [details] [diff] [review]
patch as pushed

Discussed the perms that should be used with Mossop and decided upon 0660
You need to log in before you can comment on or make changes to this bug.