1528320 - XDR-decoding can produce incorrect track-record-replay-progress flag

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

When a script is XDR-decoded, its immutable flags (including whether to track record/replay progress) are set to those of the script which did the XDR-encoding. If the XDR-encoded script and XDR-decoded script are in processes with different values for IsRecordingOrReplaying(), we can get either incorrect behavior (progress values not being tracked when they should be) or crashes at null (calling a non-existent function pointer to get the progress counter address). I think this is causing some recent crashes.

The attached patch fixes this by resetting the flag after setting the immutable flags during XDR-decoding.

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 9044237 [details] [diff] [review]
patch

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

Hm can you make this a mutable flag? Then we need to copy it in CopyScript but we don't need to change XDR and I think it's a bit more idiomatic.

Comment 2

[Jan de Mooij [:jandem]]

And sorry, I should have realized this :/

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch

Updated patch. I don't think this needs to be updated in CopyScript, because CopyScript doesn't change the script's source object.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #3)

I don't think this needs to be updated in CopyScript, because CopyScript doesn't change the script's source object.

CopyScript doesn't copy the mutable flags so if we don't copy the flag explicitly we introduce a difference in behavior.

Thinking about it more, we should also fix the Ion code here to keep TSan from complaining:

https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/js/src/jit/shared/CodeGenerator-shared.cpp#124

Can you cache this flag on the CompileInfo?

Comment 5

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #4)

(In reply to Brian Hackett (:bhackett) from comment #3)

I don't think this needs to be updated in CopyScript, because CopyScript doesn't change the script's source object.

CopyScript doesn't copy the mutable flags so if we don't copy the flag explicitly we introduce a difference in behavior.

CopyScript operates on a destination script that went through CreateEmptyScriptForClone(), which calls JSScript::Create, which sets the TrackRecordReplayProgress flag. The source object used in this path (which affects whether the flag is set) is the one which the script will ultimately have, so the flag should already be set correctly by the time we get to CopyScript.

Thinking about it more, we should also fix the Ion code here to keep TSan from complaining:

https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/js/src/jit/shared/CodeGenerator-shared.cpp#124

Can you cache this flag on the CompileInfo?

I guess so, but I'm starting to feel that the original patch I wrote is the best fix here.

Comment 6

[Brian Hackett [Laid off!]]

Comment on attachment 9044237 [details] [diff] [review]
patch

Asking r? for this patch again.  This crash is affecting users and it would be nice to get this fix into the tree, which is the most direct way of addressing the problem.

I also feel that despite modifying XDR, this is the fix which best characterizes the issue being addressed.  TrackRecordReplayProgress is an immutable property of a script, and should be in its immutable flags.  The issue is that unlike other immutable flags on the script, it is not valid to save/restore to a script in another process via XDR.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 9044237 [details] [diff] [review]
patch

Ted is doing a lot of XDR/CopyScript cleanup these days so I'll forward to him.

Comment 8

[Ted Campbell [:tcampbell]]

In Bug 1525505, we fixed the issue of calling JSScript::Create with a bogus SSO. I believe that now that this is done, the best fix for this recordreplay bug is to simply move the |TrackRecordReplayProgress| flag to be a MutableFlag. The JSScript::Create function will compute the flag based on the SSO each time, and it will not be unnecessarily copied by XDR.

I'd like to maintain that ImmutableFlags are flags that are XDR-safe and be part of the input of an eventual pure parsing function.

In Bug 1525924, I've reworked the XDRScript code.
In Bug 1528964, I've reworked the CopyScript code.

Comment 9

[Ted Campbell [:tcampbell]]

Brian, with Bug 1525502 landed, does simply changing TrackRecordReplayProgress to MutableFlags fix the bug?

Comment 10

[Brian Hackett [Laid off!]]

Attachment: patch

This patch makes TrackRecordReplayProgress a mutable flag, though I hesitate to call it simple.

Comment 11

[Ted Campbell [:tcampbell]]

Comment on attachment 9045449 [details] [diff] [review]
patch

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

Thanks for making the change! I mistakenly assumed the MutableFlag support in JIT was already implemented. This will also save me from introducing more regressions here as I wrangle JSScript/LazyScript/XDRScript/CopyScript to stop having subtle variations in behaviour.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/133c2fc80e9c
Ensure correct record/replay progress flag after XDR-decoding a script, r=tcampbell.

Comment 13

[Arthur Iakab [arthur_iakab]]

Backed out changeset 133c2fc80e9c (bug 1528320) for causing multiple build bustages on BaselineCompiler.cpp CLOSED TREE

Backout revision https://hg.mozilla.org/integration/mozilla-inbound/rev/d0609ea11a18975a9de2e5e5650f54a53dff4514

Failed push https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&revision=133c2fc80e9cbd47a8c03b6054e0458a0d67d211

Log failure https://treeherder.mozilla.org/logviewer.html#?job_id=229533634&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=229533645&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=229533694&repo=mozilla-inbound

:bhackett could you please take a look?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/241d2964ae96
Ensure correct record/replay progress flag after XDR-decoding a script, r=tcampbell.

Comment 15

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/241d2964ae96