1295775 - Sampling during compaction may access invalid JSScript*s

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

The sampler from the Gecko profiler can interrupt compaction.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Use MaybeForwarded in ProfileEntry::script.

This showed up on my try runs for bug 1263355. Needless to say I don't know how
to write a test case for this for tip.

Does this pointer also need to be updated after compaction? The only place
where ProfileEntry::trace is called is in markRuntime, which is during root
marking.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Use MaybeForwarded in ProfileEntry::script.

This should do it.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 8781756 [details] [diff] [review]
Use MaybeForwarded in ProfileEntry::script.

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

This looks like it will work to me. Forwarding to Jon for the more general question of what needs to happen for roots after CGC.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8781756 [details] [diff] [review]
Use MaybeForwarded in ProfileEntry::script.

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

Thanks for fixing this shu.

markRuntime is also called during compacting to update the roots, so the fixupAfterMovingGC part is not required.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Is the relocation overlay written atomically?

Assuming it is not: What happens if the SIGPROF is triggered when the relocation overlay is only partially written? How will MaybeForwarded and the updated pseudo stack code handle this case?

Perhaps, this is a naive question, in which case I would love an explanation as to why that is :)

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #5)
> Is the relocation overlay written atomically?
> 
> Assuming it is not: What happens if the SIGPROF is triggered when the
> relocation overlay is only partially written? How will MaybeForwarded and
> the updated pseudo stack code handle this case?
> 
> Perhaps, this is a naive question, in which case I would love an explanation
> as to why that is :)

You're right. From our IRC convo I don't have a better solution than AutoSuppressProfilerSampling.

The moving is also a problem for the JitcodeGlobalTable. Sampling must be suppressed from the time the first JSScript is moved until both the SPS stack and JitcodeGlobalTable are forwarded. :(

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Suppress sampling during compaction.

In light of fitzgen's comment 5, I'm gonna put the hammer down until we find
something better.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/121757de3a7d
Suppress sampling during compaction. (r=djvj)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/121757de3a7d