58668 - Registry APIs should use NSPR file i/o NOT C runtime

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect, P3)

Description

[Henk Pol]

Using the 27-10 build...
When I start Warpzilla, I get the following crsh 

11-01-2000  01:13:59  SYS3175  PID 0075  TID 0001  Slot 008e
C:\WARPZILLA\BIN\MOZILLA.EXE
c0000005
000188ac
P1=00000001  P2=000188ac  P3=XXXXXXXX  P4=XXXXXXXX
EAX=01040590  EBX=00000000  ECX=00000000  EDX=00000000
ESI=00000000  EDI=00000000
DS=0053  DSACC=f0f3  DSLIM=ffffffff
ES=0053  ESACC=f0f3  ESLIM=ffffffff
FS=150b  FSACC=00f3  FSLIM=00000030
GS=0000  GSACC=****  GSLIM=********
CS:EIP=005b:000188ac  CSACC=f0df  CSLIM=ffffffff
SS:ESP=0053:010403e4  SSACC=f0f3  SSLIM=ffffffff
EBP=01040414  FLG=00012212


I've tried with a new profile but keep getting the same crack...

Comment 1

[Barry Marshall]

What OS/2 level, fixpak, graphics card, etc. are you using for this?

Have you tried deleting the \OS2\MOZILLA directory as well as your Mozilla's BIN 
directory?

(Just looking to flesh out the bug a bit).

Comment 2

[Dawn Endico]

ccing mkaply, the guardian angel of os2 bugs

Comment 3

[Henk Pol]

>What OS/2 level, fixpak, graphics card, etc. are you using for this?
I'm on WSEB FP1 with build14_059 with a Matrox G200 8 MB and using the
latest beta 35 of SDD. I have 128 MB of ram and a mixed IDE- SCSI 
system...

>Have you tried deleting the \OS2\MOZILLA directory as well as your
>Mozilla's BIN  directory?

I install every build in a new fresh Warpzilla dir and make a new
\OS2\Mozilla dir as well.
These build has run but suddenly when I started it a got the above
mentioned crash. I deleted the profile from \os2\mozilla and started 
again with the same result.

Comment 4

[Mike Kaply [:mkaply]]

Is it possible you have a copy of mozilla already running?

Could you reboot and then try it?

We just found a trap in mozilla.exe starting mozilla when another mozilla is 
running.

Comment 5

[Henk Pol]

Mike,

I've rebooted it and I can start/run Warpzilla again.
I remember that sometimes after a crash a kind of VIO window stays
open but there is no Warpzilla running..
So perhaps in the backgrounf there was one...
I'm happy again :-)

Thnx for all the good work...

Comment 6

[Mike Kaply [:mkaply]]

The bug here is that if you type "mozilla" after mozilla is already started, you 
trap.

We either need the up check that Win has, or fix the trap.

Comment 7

[Mike Kaply [:mkaply]]

I believe this bug is really XP, although OS/2 probably shows it off better.

In nsAppRunner, if NS_InitXPCOM fails, there is an assert and then the browser
continues to load.

I would think that NS_InitXPCOM failing would be a reason to bail out of
starting the browser.

Comment 8

[Mike Kaply [:mkaply]]

Back to me.

Makefile changes to avoid using fopen in registry.

It's probably a bug that they use fopen.

Comment 9

[Mike Kaply [:mkaply]]

Attachment: New Makefile.in for libreg to remove buffered IO for OS/2

Comment 10

[cls]

The Makefile changes look ok to me (r=cls). Maybe dveditz knows why fopen is
being used instead of a NSPR wrapper or some other call.

Comment 11

[Mike Kaply [:mkaply]]

Fix checked in.

I'll discuss separately with dveditz why fopen is being used in reg.

Comment 12

[Mike Kaply [:mkaply]]

BAD BAD fix.

This clobbers performance on network drives.

The right fix is to use NSPR calls instead of file system calls.

I have a patch.

Comment 13

[Mike Kaply [:mkaply]]

Attachment: Changes to libreg to use NSPR file I/O rather than C

Comment 14

[Mike Kaply [:mkaply]]

I tested this and it works great for us. Obviously is is a big change since it 
impacts component.reg so it needs to be tested on other plats.

I'll be testing Windows.

Comment 15

[Daniel Veditz [:dveditz]]

r=dveditz

Comment 16

[Brendan Eich [:brendan]]

-                if ( !fseek( fd, 0, SEEK_END ) )
+                if ( PR_Seek( fd, 0, SEEK_END ) >= 0)

Use PR_SEEK_END with PR_SEEK, although the well-known values are the same.

+                    file->readOnly = (flags == PR_RDONLY);

Shouldn't that be (flags & PR_RDONLY), if only for future-proofing?

-                if ( fseek( file->fd, file->fpos, SEEK_SET ) == 0 )
+                if ( PR_Seek( file->fd, file->fpos, SEEK_SET ) >= 0 )

Use PR_SEEK_SET.  There are two more such cases after this one.

-#define XP_FileOpen(path, mode)         bufio_Open((path), (mode))
+#define XP_FileOpen(path, mode)         bufio_Open((path), mode)

Confusion: bufio_Open takes three args now, a la PR_Open.  How can this work?

/be

Comment 17

[Mike Kaply [:mkaply]]

brendan: I'll make the changes you mention.

As far as the bufio_Open goes, here is how XP_FileOpen gets called:

*fh = XP_FileOpen(path, XP_FILE_UPDATE_BIN);

XP_FILE_UPDATE_BIN gets defined to either one param or two params depending on 
whether or not you are using C runtime file I/O or NSPR file I/O.

So with C runtime file I/O it is:

#define XP_FILE_UPDATE_BIN       "r+"

But with NSPR it looks like:

#define XP_FILE_UPDATE_BIN       PR_RDWR|PR_CREATE_FILE, 0644

Which is two parameters.

By removing the parenthesis, we cause the macro to actually place two additional 
parameters in the call to bufio_Open rather than one.

For a similar situation, see:

http://lxr.mozilla.org/seamonkey/source/modules/libreg/src/vr_stubs.h#147

Comment 18

[Mike Kaply [:mkaply]]

One more thing I found.

Can 

http://lxr.mozilla.org/seamonkey/source/modules/libreg/src/nr_bufio.c#140

Through line 168, as well as the XP_MAC include of Errors.h be removed because 
of this change?

NSPR should be setting the right error now.

Comment 19

[Brendan Eich [:brendan]]

mkaply: yeah, now that you're using NSPR i/o instead of stdio, no need to set
NSPR error -- that whole else-containing-switch can go, it seems to me.  How
about a new patch?

/be

Comment 20

[Mike Kaply [:mkaply]]

Attachment: Latest and greatest diff

Comment 21

[Brendan Eich [:brendan]]

sr=[e-mail]

/be

Comment 22

[Mike Kaply [:mkaply]]

Fix checked in.

Comment 23

[Conrad Carlen (not reading bugmail)]

This completely breaks the Mac. My build from this morning won't even start up.
Problem is CreateMacPathFromUnixPath() in macio.c is wrong - opening any file
specified with a full path (as the registry file is) will fail.

Comment 24

[Mike Kaply [:mkaply]]

Conrad,

do you have any idea what it would take to fix CreateMacPathFromUnixPath()?

This change would not be asy to #ifdef for Mac, and I would prefer to fix the 
problems rather than back it out.

Comment 25

[Conrad Carlen (not reading bugmail)]

Looking into it. Problem is determining whether anything else in NSPR is 
depending on the current behavior of CreateMacPathFromUnixPath(). I hope not but
I'm looking.

Comment 26

[Simon Fraser [no longer active]]

Something that needs to be tested once this is done is running Mozilla when 
either the hard drive name, or one of the folders in the path to the app contains 
slashes ('/') and other path-sensitive characters. It works before this patch, 
and we need to ensure that it continues to work.

Comment 27

[Simon Fraser [no longer active]]

The registry location path that is being passed in to libreg comes from an 
nsIFile::GetPath call. Note that nsIFile.idl has comments above the path 
attribute that specifically state that this path is not guaranteed to work with 
NSPR calls (for the reason that's it's a native path, where NSPR requires Unix 
paths).

Naive Max -> Unix path conversions (e.g. the one that is done here:
<http://lxr.mozilla.org/seamonkey/source/modules/libreg/src/vr_stubs.c#294>
and here:
<http://lxr.mozilla.org/seamonkey/source/modules/libreg/src/vr_stubs.c#353>
will not work, because they do not allow for Mac folders that contain slashes.
For this reason, code that uses 'globalRegName' is probably already broken.

It seems that the best way to fix this bug would be to pass nsIFiles into libreg, 
and for it to use nsILocalFile::openNSPRFileDesc(). However, this would add 
dependencies from libreg to XPCOM. Other suggestions?

Comment 28

[Daniel Veditz [:dveditz]]

Using nsIFile in libreg is bug 15752.

Comment 29

[Brendan Eich [:brendan]]

Apologies to all for my failure to note the GetPath => NSPR PR_OpenFile data
flow in conjunction with GetPath's well-commented native-ness.

mkaply, are you up for tackling this still?  It may entail hairy factoring or
linkage cyclic dependency hacking so that libreg can both build standalone, and
use xpcom/io interfaces, without breaking xpcom/components' use of libreg.

Anyone have a good idea for the right fix?  I'm suffering from better-is-better
right now in thinking that xpcom/io could become standalone, so that the
dependency graph looks like:

        xpcom/io --> xpcom/{base,ds,other-most-primitive-stuff}
           ^         ^          ^
           |         |          |
           |         |          |
        libreg <-- xpcom/components

No cycles here, but I haven't analyzed all the dependencies.  Cc'ing xpcom
buddies.  We should take this to the mozilla.xpcom newsgroup if it develops
(quickly, in this bug) that we want to do something like this.

/be

Comment 30

[Brendan Eich [:brendan]]

Shaver may have a better idea, too.

/be

Comment 31

[Daniel Veditz [:dveditz]]

At some point someone converted libreg into a static library on unix and simply 
linked it into xpcom. This could be done on mac and win which would be 
another way to break the cycle. It would also allow libreg to use non-fatal 
NS_ASSERTION type calls rather than fatal PR_ASSERT() (there's a bug on that) 
which would make some folks extremely happy.

I've had the code changes for making libreg a static lib on windows sitting in 
my tree but never found time to finish it up (and no Mac to finish the job).

Comment 32

[Kathleen :Brade]

add dependency

Comment 33

[Conrad Carlen (not reading bugmail)]

dveditz:
> I've had the code changes for making libreg a static lib on windows sitting in
> my tree but never found time to finish it up (and no Mac to finish the job).
What do you need on the Mac in terms of project creation/build script tweaking?
Can I be of help? Can you post your makefile changes on bug 15752?

Comment 34

[Conrad Carlen (not reading bugmail)]

Actually, bug 15752 may not be the place for that. File a new bug on making reg
lib a static lib?

Comment 35

[Daniel Veditz [:dveditz]]

Why don't I attach it to bug 40464 which prompted the work?

Comment 36

[Vidar Haarr (not reading bugmail)]

What is the status here ?

Comment 37

[Mike Kaply [:mkaply]]

Attachment: OS/2 specific fix

We have recently found that this problem is causing some other bugs, in
particular the inability to use MOZ_NO_REMOTE on OS/2 and crashes using the new
turbo code that restarts the browser.

So my fix is to take code that we did for MORK and put it in libreg.

Essentially on OS/2 we use sdopen instead of fopen and map the descriptor using
fdopen.

This fixes all problems related to this issue, since the issue was that fopen
was being called twice.

Comment 38

[jhp (no longer active)]

Comment on attachment 94041 [details] [diff] [review]
OS/2 specific fix

r=pedemont

Comment 39

[Alec Flett]

Comment on attachment 94041 [details] [diff] [review]
OS/2 specific fix

sr=alecf

Comment 40

[Mike Kaply [:mkaply]]

I'm moving this to normal and taking it off the OS/2 radar.

The OS/2 blocker issue is resolved.

The remaining issue is the fact that these APIs shouldn't be using C runtime
file I/O calls.

As a matter of fact, someone should go through Mozilla and remove every fopen
they find and make it use NSPR.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

mkaply, should comment #40 be filed as a new bug?