1303779 - NSPR changes for web replay

Status: RESOLVED WONTFIX

(NSPR :: NSPR, defect)

Description

[Brian Hackett [Laid off!]]

This bug has changes to NSPR which are needed by Web Replay (bug 1207696).

Comment 1

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Add record/replay NSPR interface.

This patch adds a minimal record/replay interface to NSPR, for use by NSPR and NSS.  This allows NSPR to initialize the record/replay system, to test whether the process is recording or replaying, and to call interfaces in the record/replay system.  The implementation of the record/replay system lives in XUL; if the process isn't actually linking against XUL then setting the record/replay environment variables will not do anything.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Ensure that PL hashtables have consistent iteration order when recording/replaying.

When recording or replaying, call interfaces in the record/replay system which manage custom callbacks for PL hash tables (these callbacks produce consistent hash numbers between recording and replay).

Comment 3

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Atomics interface changes.

Instrument uses of the PR_ATOMIC_* macros so that atomic operations execute in a consistent order when recording or replaying.  This instrumentation will never do anything when NSPR is being not being used with XUL, and would be nice to avoid with a compiler option when that is the case, but I don't know the best way of doing that.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: Part 4 - Don't record NSPR thread/lock-management atomics.

Don't record some atomic quantities used internally for condvars and monitors.  These aren't related to ensuring consistent record/replay, and if the condvar/monitor is not itself recorded then we don't want to record this activity either.

Comment 5

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8792560 [details] [diff] [review]
Part 1 - Add record/replay NSPR interface.

Review of attachment 8792560 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/external/nss/nss.symbols
@@ +11,5 @@
>  # excluded on Windows, but it doesn't matter because the symbols are already
>  # exported in NSPR (Windows peculiarity).
>  PR_*
>  PL_*
> +gPR*

that's an NSS change

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #5)
> Comment on attachment 8792560 [details] [diff] [review]
> Part 1 - Add record/replay NSPR interface.
> 
> Review of attachment 8792560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/external/nss/nss.symbols
> @@ +11,5 @@
> >  # excluded on Windows, but it doesn't matter because the symbols are already
> >  # exported in NSPR (Windows peculiarity).
> >  PR_*
> >  PL_*
> > +gPR*
> 
> that's an NSS change

Which bug should this change go into?  When I looked at the revision log for this file it seemed to be part of the m-c build process, rather than being tied to any particular NSS release (but I don't know this code at all really).

Comment 7

[Franziskus Kiefer [:franziskus]]

(In reply to Brian Hackett (:bhackett) from comment #6)
> (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #5)
> > Comment on attachment 8792560 [details] [diff] [review]
> > Part 1 - Add record/replay NSPR interface.
> > 
> > Review of attachment 8792560 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: config/external/nss/nss.symbols
> > @@ +11,5 @@
> > >  # excluded on Windows, but it doesn't matter because the symbols are already
> > >  # exported in NSPR (Windows peculiarity).
> > >  PR_*
> > >  PL_*
> > > +gPR*
> > 
> > that's an NSS change
> 
> Which bug should this change go into?  When I looked at the revision log for
> this file it seemed to be part of the m-c build process, rather than being
> tied to any particular NSS release (but I don't know this code at all
> really).

yeah, good question. Since it's going to central directly you have to put it in a separate patch anyway. Then in here should be fine I guess.

Comment 8

[Wan-Teh Chang]

Comment on attachment 8792560 [details] [diff] [review]
Part 1 - Add record/replay NSPR interface.

Review of attachment 8792560 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Brian: thank you for the NSPR patches, and sorry for the late review.

I read all four NSPR patches over the weekend. Although I don't fully
understand them, I hope I roughly understand what each patch does.

1. I hope you can find a solution that does not require modifying
NSPR and NSS. Although many Mozilla developers contribute to NSPR
and NSS, we should treat NSPR and NSS as third-party code that
Mozilla doesn't have full control of. I hope web replay can be
implemented without modifying all third-party code used in Mozilla.

2. The reason I think this NSPR patch is not appropriate is that
NSPR would merely play the role of answering the question "are we
recording or replaying?" but the implementation of this feature
would be outside NSPR. This feature will be implemented by a
particular client of NSPR (Mozilla/Firefox) and won't be available
to other NSPR clients.

Comment 9

[Wan-Teh Chang]

Comment on attachment 8792563 [details] [diff] [review]
Part 2 - Ensure that PL hashtables have consistent iteration order when recording/replaying.

Review of attachment 8792563 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Brian: for this PLHashTable patch, I hope the following alternative
solution will work.

Identify the PLHashTable objects in Mozilla/Firefox that need consistent
hash numbers between recording and replay, and create those PLHashTable
objects by passing a custom |keyHash| function to PL_NewHashTable().

If this is too much work or too error-prone, then I suggest adding a
wrapper function of PL_NewHashTable() and changing Mozilla/Firefox
globally to use the wrapper function instead of PL_NewHashTable().
The wrapper function will set up for recording and replay, if
necessary, and then call PL_NewHashTable().

Since your patch also modifies PL_HashTableDestroy(), we may also
need a wrapper function of PL_HashTableDestroy().

Comment 10

[Wan-Teh Chang]

Comment on attachment 8792569 [details] [diff] [review]
Part 3 - Atomics interface changes.

Review of attachment 8792569 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Brian: I don't fully understand the need for executing atomic operations in
a consistent order when recording or replaying, so I don't have a suggestion
for this patch.

Although most programmers would love people to use their code, I am willing
to ask you to replace calls to NSPR atomic functions with Mozilla's equivalent
so that pratom.h and pratom.c don't need to be modified. Switching to Mozilla's
atomics seems worthwhile in its own right because they allow memory order to
be specified.

What do you think of this proposal?

Comment 11

[Wan-Teh Chang]

Comment on attachment 8792571 [details] [diff] [review]
Part 4 - Don't record NSPR thread/lock-management atomics.

Review of attachment 8792571 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Brian: just wanted to note that I've also read this patch.
This patch depends on the "Part 3 - Atomics interface changes"
patch. So we can deal with these two patches together.

Comment 12

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 13

[Brian Hackett [Laid off!]]

Thanks to other m-c changes (mainly NSPR not being initialized until after main() starts), Web Replay no longer requires changing NSPR.