1465452 - Web Replay: Gecko recording tweaks

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, defect)

Description

[Brian Hackett [Laid off!]]

This bug has changes needed in Gecko to specify whether mutexes or atomic values should be recorded, and adjustments in specific places to record or not record such activity.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Allow platform mutexes to specify whether they are recorded.

Mutexes and monitors have their behavior recorded by default, but this can be changed in the constructor.  This is pretty similar to part 4b of bug 1207696 (some parts of which still exist in the codebase and have had their review carried forward).

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Allow nsISupports subclasses to record/replay their refcount changes.

nsISupports threadsafe refcounts are not recorded by default, but this can be overridden with the NS_DECL_THREADSAFE_ISUPPORTS_WITH_RECORDING macro.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Avoid recording some background hang monitor state.

Background hang monitor state can be accessed at non-deterministic points (during the interrupt callback I think).

Comment 4

[Brian Hackett [Laid off!]]

Attachment: Part 4 - Don't sort image loader requests and frames when recording/replaying.

This is pretty similar to part 5f of bug 1207696, but the associated image loader code has changed a fair amount and the fix is also cleaner now.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: Part 5 - Don't record some jemalloc atomics.

Malloc can be called at non-deterministic points when recording/replaying.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: Part 6 - Don't record assertion crashing flag.

If the assertion crash flag is recorded then we can another error when an assertion fails that obscures the real problem.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: Part 7 - Don't record a profiler atomic.

Profiler flags can be accessed during GC and other places where recorded events are not allowed.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: Part 8 - Don't record logging state.

Logging code can be accessed during GC and other places where recorded events are not allowed.

Comment 9

[Brian Hackett [Laid off!]]

Attachment: Part 9 - Allow warnings and errors to be printed without affecting the recording.

Passing through thread events when NS_WARNING and errors are printed allows these methods to be freely called in the GC and other places where recorded events are not allowed.

Comment 10

[Brian Hackett [Laid off!]]

Attachment: Part 10 - Don't record atom table atomic.

I think this atomic is accessed during GC.

Comment 11

[Brian Hackett [Laid off!]]

Attachment: Part 11 - Record refcount changes for pipe input/output streams.

Pipe input and output streams have destructors that can perform recorded events, and if their refcount changes are not recorded then the thread which destroys them can vary between recording and replay.

Comment 12

[Brian Hackett [Laid off!]]

Attachment: Part 12 - Record refcount changes for abstract threads.

Some abstract thread subclasses have destructors that can perform recorded events, and if their refcount changes are not recorded then the thread which destroys them can vary between recording and replay.

Comment 13

[Brian Hackett [Laid off!]]

Attachment: Part 13 - Avoid recording some timer thread state.

Comment 14

[Brian Hackett [Laid off!]]

Sorry, forgot the comment on the last patch.  I think timer thread state can be accessed during the GC.

Comment 15

[Brian Hackett [Laid off!]]

Attachment: Part 14 - Record refcount changes for runnables.

Some runnable subclasses have destructors that can perform recorded events, and if their refcount changes are not recorded then the thread which destroys them can vary between recording and replay.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 8981859 [details] [diff] [review]
Part 3 - Avoid recording some background hang monitor state.

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

Maybe the rationale ("Background hang monitor state can be accessed at non-deterministic points (during the interrupt callback I think).") should be expanded slightly into a comment somewhere in this file?

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 8981862 [details] [diff] [review]
Part 6 - Don't record assertion crashing flag.

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

"then we can another error when an assertion fails"?  Does that mean to say that we can record two errors, and the second one obscures the first?  Or something else?

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 8981874 [details] [diff] [review]
Part 14 - Record refcount changes for runnables.

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

I think I believe that this is necessary, but "...then the thread which destroys them can vary between recording and replay." sounds completely bewildering.  How does that even happen?

I guess that means if we're not sequencing refcount changes on runnables, we might not be running the refcount changes in the same order between record and replay, and therefore thread 1 could see the refcount go to zero during record, but thread 2 sees the refcount go to zero in the replay.  Is that a correct understanding of the situation?

Comment 19

[Nathan Froyd [:froydnj]]

Comment on attachment 8981871 [details] [diff] [review]
Part 13 - Avoid recording some timer thread state.

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

Some comment about why this is necessary would be great.  (Actually, some comment about why we're DontPreserve'ing things in each patch in this bug wouldn't be a bad idea, similar to the way we attempt to have people using Relaxed memory ordering with atomics justify themselves in the code.)

Timers during GC sounds very plausible.

Comment 20

[Brian Hackett [Laid off!]]

(In reply to Nathan Froyd [:froydnj] from comment #17)
> Comment on attachment 8981862 [details] [diff] [review]
> Part 6 - Don't record assertion crashing flag.
> 
> Review of attachment 8981862 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> "then we can another error when an assertion fails"?  Does that mean to say
> that we can record two errors, and the second one obscures the first?  Or
> something else?

If an assertion fails when we are recording or replaying and are in the GC or another place where recorded events are not allowed, then touching the crashing flag will cause a separate fatal error to be reported by the record/replay system, unrelated to the original failing assertion.  Sorry about the garbled description.

Comment 21

[Brian Hackett [Laid off!]]

(In reply to Nathan Froyd [:froydnj] from comment #18)
> Comment on attachment 8981874 [details] [diff] [review]
> Part 14 - Record refcount changes for runnables.
> 
> Review of attachment 8981874 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I believe that this is necessary, but "...then the thread which
> destroys them can vary between recording and replay." sounds completely
> bewildering.  How does that even happen?
> 
> I guess that means if we're not sequencing refcount changes on runnables, we
> might not be running the refcount changes in the same order between record
> and replay, and therefore thread 1 could see the refcount go to zero during
> record, but thread 2 sees the refcount go to zero in the replay.  Is that a
> correct understanding of the situation?

Yes, this is the problem which this patch and the other ones on nsISupports refcounts are fixing.  In principle we should be able to record refcount changes for all (or almost all) nsISupports, but this would add a lot of recording overhead and in most cases is unnecessary, so we default to not recording the changes and then doing spot fixes for classes where that is a problem.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff685ebac3e
Part 1 - Allow platform mutexes to specify whether they are recorded, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/16226b867568
Part 2 - Allow nsISupports subclasses to record/replay their refcount changes, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/894b187d288f
Part 3 - Avoid recording some background hang monitor state, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b542f98c02c
Part 5 - Don't record some jemalloc atomics, r=njn.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef7ed9b4ee49
Part 6 - Don't record assertion crashing flag, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/554c3f761c64
Part 7 - Don't record a profiler atomic, r=njn.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b091c71170a
Part 8 - Don't record logging state, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/25957aa95ba3
Part 10 - Don't record atom table atomic, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fc6404e8d2a
Part 11 - Record refcount changes for pipe input/output streams, r=erahm.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f01abf3b9dd7
Part 12 - Record refcount changes for abstract threads, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebe594fec36d
Part 13 - Avoid recording some timer thread state, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8f82b41f1af
Part 14 - Record refcount changes for runnables, r=froydnj.

Comment 23

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/7ff685ebac3e
https://hg.mozilla.org/mozilla-central/rev/16226b867568
https://hg.mozilla.org/mozilla-central/rev/894b187d288f
https://hg.mozilla.org/mozilla-central/rev/7b542f98c02c
https://hg.mozilla.org/mozilla-central/rev/ef7ed9b4ee49
https://hg.mozilla.org/mozilla-central/rev/554c3f761c64
https://hg.mozilla.org/mozilla-central/rev/3b091c71170a
https://hg.mozilla.org/mozilla-central/rev/25957aa95ba3
https://hg.mozilla.org/mozilla-central/rev/2fc6404e8d2a
https://hg.mozilla.org/mozilla-central/rev/f01abf3b9dd7
https://hg.mozilla.org/mozilla-central/rev/ebe594fec36d
https://hg.mozilla.org/mozilla-central/rev/d8f82b41f1af

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/654209457989
Part 4 - Don't sort image loader requests and frames when recording/replaying, r=aosmond.
https://hg.mozilla.org/integration/mozilla-inbound/rev/15e1646fafb9
Part 9 - Allow warnings and errors to be printed without affecting the recording, r=froydnj.

Comment 25

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/654209457989
https://hg.mozilla.org/mozilla-central/rev/15e1646fafb9