353899 - PR_CallOnce / PR_CallOnceWithArg do not set NSPR error code if once->initialized is TRUE and once->status is PR_FAILURE

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Julien Pierre]

NSPR should set an error code for this case. If we can't record the error code from the initial failure in the PRCallOnceType, we should just set an arbitrary error code. But we should not leave the error code as 0.

Comment 1

[Julien Pierre]

Attachment: Set the error the second time around to PR_PREVIOUSLY_INITIALIZED_ERROR

Comment 2

[Julien Pierre]

Setting P2. Looks like we should have a new target for NSPR 4.6.4. I will file a bug to request one.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

ANy hope of getting it to repeat the original error code 
instead of setting a new one?

Comment 4

[Julien Pierre]

Nelson,

We can't store the error code in the PRCallOnceType unfortunately - there is no room, and we can't extend it without breaking binary compatibility. So the only hope would be for some external storage, maybe by building a global hash table of pointers to error codes. But that would make PR_CallOnce much more expensive for failure cases. Do you think it's really necessary ? I would guess that most applications will either log the error code all the time or never. So, for diagnosis purposes, one could look at the first error.

Comment 5

[Wan-Teh Chang]

pthread_once doesn't return the error code of the init routine
either.  In fact, pthread_once can't even tell if the init routine
succeeded or failed because the init routine returns void:
http://www.opengroup.org/onlinepubs/009695399/functions/pthread_once.html

If the error code is important, the init routine can save it in
a global variable.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #5)
> pthread_once doesn't return the error code of the init routine
> either.  In fact, pthread_once can't even tell if the init routine
> succeeded or failed because the init routine returns void:

What is the relevance of those observations to PR_CallOnce?
PR_CallOnce returns a PR_STATUS, and the once-called function does too.
It is my understanding that the status returned by PR_CallOnce reflects
the value returned by the once-called function, at least on the one 
time that it is called.

Are you saying that on some platforms, PR_CallOnce uses pthread_once,
and does not return the value from the called function?  

Is the value set in PR_SetError not returned by PR_GetError after
the once-called function returns?

Comment 7

[Wan-Teh Chang]

The error code set by PR_CallOnce is not documented:
http://www.mozilla.org/projects/nspr/reference/html/prinit.html#15926
http://developer.mozilla.org/en/docs/PR_CallOnce

Since PR_CallOnce is modeled after pthread_once, I wanted
to know what pthread_once does when the init routine fails.
I also pointed out that callers of PR_CallOnce can work
around the lack of error code by saving the error code in
a global variable.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Seems to me that if the once-called function fails and return PR_FAILURE,
it should have set the NSPR error code, and PR_CallOnce should not change
it in that case.  OTOH, if PR_CallOnce itself fails, without calling the 
once-called function, then it should set some error code that ideally is
distinguishable from those the once-called function would have set.  

I seem to recall that the CallOnce structure contains a word that saves
the success/failure state of the once-called function.  Perhaps it 
could contain the error code returned from the first call to PR_CallOnce
for that callonce structure, instead, or PR_FAILURE IFF the CallOnce 
function didn't set a non-zero error code.

Comment 9

[Julien Pierre]

Nelson,

Re: comment 8,

1) PR_CallOnce itself never fails currently ;) It always returns once->status .
The only two APIs called that could fail are PR_WaitCondVar / PR_NotifyAllCondVar, but their return value is not currently being checked .

2) PRCallOnceType is a public structure defined as follows :

typedef struct PRCallOnceType {
    PRIntn initialized;
    PRInt32 inProgress;
    PRStatus status;
} PRCallOnceType;

The third field stores the result of the function. See the implementation for PR_CallOnce :

  once->status = (*func)();

The once functions are defined to return PRStatus, not PRErrorCode. PRStatus is an enum whose only legal values are PR_FAILURE and PR_SUCCESS . IMO, changing the type of once->status to accept other values breaks binary compatibility . Some programs may look at the value of once->status and not expect other values, such as the NSPR error code, that are currently illegal.

The first two fields are currently for use by PR_CallOnce / PR_CallOnceWithArg . Even though they are part of a public structure, hopefully nobody else relies on any specific value for them. But I'm not sure we can make that assumption. If we can, we could change the meaning of the field and possibly find space for the error code in them. We need inProgress to be atomically-settable with PR_AtomicSet, so I think there is no hope to compress that field. 

On the other hand, once->initialized currently uses only 1 bit effectively - the values 0 and 1. But that could be changed in the implementation or PR_CallOnce / PR_CallOnceWithArg, if we can assume that nobody else looks at once->initialized. This might leave enough bits to store the PRErrorCode .
Unfortunately, PRIntn is defined to be an int of 16-bits or greater, so there may be some platforms, at least theoritically, on which the PRErrorCode doesn't fit, since PRErrorCode is defined to be a PRInt32 .

So, I think that says that we can't fit the PRErrorCode anywhere in the existing definition of PRCallOnceType . I would rather change that definition when NSPR 5 comes around than add more hacks to NSPR 4, for example, to store the initial PRErrorCode of the once function in a hash table with &once as the key .

Comment 10

[Julien Pierre]

Hmm, I'm not sure how I ended up changing the assignee with the previous comment. It was accidental. Back to me.

Comment 11

[Wan-Teh Chang]

prerr.h is a generated file. We need to change prerr.et,
do "gmake build_prerr", and check in the new prerr.h,
prerr.c, prerr.properties, and prerr.et.

Also it's better to say else if { } instead of
else { if { } }.

Comment 12

[Wan-Teh Chang]

Nelson, please set the target milestone of this bug.
I think the fix of this bug can wait until NSPR 4.7.
For now I left it targeted at a 4.6.x patch release.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh took this bug when Julien left.  Now that Julien's back, 
I'm giving this bug back to him  

Wan-Teh, please complete this review.

Comment 14

[Julien Pierre]

Reassigning target since 4.6.7 is already RTM.

Comment 15

[Wan-Teh Chang]

Comment on attachment 239740 [details] [diff] [review]
Set the error the second time around to PR_PREVIOUSLY_INITIALIZED_ERROR

Sorry about the delay in review.

I recommend we make this change for NSPR 4.7 only.
The original NSS bug has been fixed without requiring
this new NSPR feature.

Two minor problems.

1. Rename the error code PR_CALL_ONCE_ERROR.

2. prerr.h is a generated file.  To add a new error code,
start with mozilla/nsprpub/pr/src/misc/prerr.et.  Do
"gmake build_prerr" in that directory to generate
prerr.h, prerr.c, and prerr.properties.  Move the
generated prerr.h to mozilla/nsprpub/pr/include.
Finally, restore the license header in the generated
files.

Comment 16

[Julien Pierre]

Attachment: rename error code, change source files

Comment 17

[Wan-Teh Chang]

Comment on attachment 264299 [details] [diff] [review]
rename error code, change source files

r=wtc.

In pr/src/misc/prerr.et

> ec PR_LIBRARY_NOT_LOADED_ERROR, "The library is not loaded"
>-
>+ec PR_CALL_ONCE_ERROR, "The one-time function was previously called and failed. Its error code is no longer available"
> ec PR_MAX_ERROR,                "Placeholder for the end of the list"

You may want to keep the blank line before the PR_MAX_ERROR line.

Comment 18

[Julien Pierre]

Thanks for the reviews, Wan-Teh. I checked the fix in to the NSPR trunk .

Checking in pr/include/prerr.h;
/cvsroot/mozilla/nsprpub/pr/include/prerr.h,v  <--  prerr.h
new revision: 3.10; previous revision: 3.9
done
Checking in pr/src/misc/prerr.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prerr.c,v  <--  prerr.c
new revision: 3.11; previous revision: 3.10
done
Checking in pr/src/misc/prerr.et;
/cvsroot/mozilla/nsprpub/pr/src/misc/prerr.et,v  <--  prerr.et
new revision: 3.8; previous revision: 3.7
done
Checking in pr/src/misc/prerr.properties;
/cvsroot/mozilla/nsprpub/pr/src/misc/prerr.properties,v  <--  prerr.properties
new revision: 3.8; previous revision: 3.7
done
Checking in pr/src/misc/prinit.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prinit.c,v  <--  prinit.c
new revision: 3.46; previous revision: 3.45
done