98068 - Add PR_CALLBACK to callback function prototypes and application code

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[(Don't use, old account) - Kai Engert]

Add PR_CALLBACK to PK11PasswordPrompt and PK11_SetPasswordFunc.

This involves changing NSS and PSM code.

I'll attach a patch. Bob, can you please review?

Comment 1

[(Don't use, old account) - Kai Engert]

I correct, not PK11_SetPasswordFunc must be corrected, but the function pointer
prototype. And I found two additional functions that might need to be changed.

This patch includes changes for PSM, which should be applied at the same time
this bug gets fixed.

Changing summary.

Comment 2

[(Don't use, old account) - Kai Engert]

Attachment: Suggested fix

Comment 3

[Robert Relyea]

r=relyea

Comment 4

[Christopher Blizzard (:blizzard)]

Comment on attachment 48075 [details] [diff] [review]
Suggested fix

sr=blizzard

Comment 5

[Stephane Saux]

Kai, should this go on the trunk?

Comment 6

[(Don't use, old account) - Kai Engert]

Yes, I talked to Bob, and I need to make sure it compiles with NSS trunk and
NSS_3_3_BRANCH before I check in. However, there were some compile problems when
I tried. I assume my problem was just caused by a temporary breakage of the NSS
tree. Haven't spent more time on this yet, will retry.

Comment 7

[(Don't use, old account) - Kai Engert]

Hmm, I wonder what I have triggered with this bug.

Bob, just to reassure before I do it. Is it correct, that I should make sure,
that all occurrences of PK11_SetPasswordFunc all over NSS are called with a
function pointer, that is of a function type that includes PR_CALLBACK?

This would result in (trivial) changes to about 20 files.

Comment 8

[(Don't use, old account) - Kai Engert]

Changing my e-mail address.

Comment 9

[Wan-Teh Chang]

Kai, only OS/2 needs PR_CALLBACK.  Are you building on
OS/2?

Have you checked in your patch?  If you need help testing
on OS/2, you can ask Julien.

Comment 10

[Kai Engert [:KaiE:]]

CC'ing wtc, as he helped me with PR_CALLBACK before.

Some days ago I had a chat with Bob about this topic. His statement was: We must
make sure that this change has no impact on our binary compatibility. Before we
check this in, we must make sure that is has no impact for those platforms where
we ship NSS in binary form.

If it is true that PR_CALLBACK is only defined on platforms where we do not ship
binaries, then it is safe to check this change in.

I have not yet checked it in.

If we agree that it is safe to do, I can check it in.

Comment 11

[Julien Pierre]

I agree that this is fine.

Comment 12

[Wan-Teh Chang]

Comment on attachment 48075 [details] [diff] [review]
Suggested fix

r=wtc.

Comment 13

[Wan-Teh Chang]

Kai Engert wrote:
> If it is true that PR_CALLBACK is only defined on platforms
> where we do not ship binaries, then it is safe to check this
> change in.

This is true.  PR_CALLBACK is only defined on WIN16 and OS/2.
We do not ship NSS binaries on these platforms.

Kai, I want to ask you again, are you building the Mozilla
client on OS/2, or did you receive this patch from an OS/2
developer?

Comment 14

[Kai Engert [:KaiE:]]

wtc: I created this patch on my own, this patch is not caused by any bug
reports.

I just came across this when I looked at the code. I remembered you said in the
past, it is important to use PR_CALLBACK, so I thought it is a good idea to have
this change.

Comment 15

[Wan-Teh Chang]

In light of Kai's answer to my question, I need to qualify
my code review.  I can only say that your patch is safe
for non-OS/2 platforms but I did not verify that those
functions are all callback functions.  They all seem like
callback functions to me though. :-)

Comment 16

[Julien Pierre]

Kai,

I could try to build this on OS/2. did you build this as part of the browser ?
Which version of NSS did you make the patch on ?

I have an OS/2 browser build as well as standalone OS/2 NSS 3.4 build , though
the later still has a lot of fixes that need to be checked in before
nss_build_all will fully work.

Comment 17

[Wan-Teh Chang]

Let's try and get this checked into NSS 3.4.  Julien, could
you checked in the NSS part of the patch?  I just reviewed
it again and think it is fine.  r=wtc.

Comment 18

[Julien Pierre]

Done

Comment 19

[Wan-Teh Chang]

Kai, you can mark this bug fixed after you check in
the PSM part of your patch.

Comment 20

[Kai Engert [:KaiE:]]

Attachment: Updated PSM part of fix

Now that the change has been checked in to NSS 3.4, we should make sure it
lands as soon as NSS 3.4 lands, but not earlier.

I therefore suggest to check it in now. In the new patch the NSS_3_4 flag
controls whether the new or old definition is used.

Comment 21

[Kai Engert [:KaiE:]]

Javi, can you please review?

Comment 22

[Javier Delgadillo]

Comment on attachment 66085 [details] [diff] [review]
Updated PSM part of fix

r=javi

Comment 23

[Christopher Blizzard (:blizzard)]

Comment on attachment 66085 [details] [diff] [review]
Updated PSM part of fix

sr=blizzard

Comment 24

[Kai Engert [:KaiE:]]

Mozilla portion checked in, fixed.