Closed Bug 1295775 Opened 4 years ago Closed 4 years ago

Sampling during compaction may access invalid JSScript*s

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: shu, Unassigned)

Details

Attachments

(1 file, 2 obsolete files)

The sampler from the Gecko profiler can interrupt compaction.
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.
Attachment #8781752 - Flags: feedback?(terrence)
This should do it.
Attachment #8781756 - Flags: review?(terrence)
Attachment #8781752 - Attachment is obsolete: true
Attachment #8781752 - Flags: feedback?(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.
Attachment #8781756 - Flags: review?(terrence) → review?(jcoppeard)
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.
Attachment #8781756 - Flags: review?(jcoppeard) → review+
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 :)
(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. :(
In light of fitzgen's comment 5, I'm gonna put the hammer down until we find
something better.
Attachment #8782174 - Flags: review?(kvijayan)
Attachment #8781756 - Attachment is obsolete: true
Summary: ProfileEntry::script() needs to MaybeForward the script → Sampling during compaction may access invalid JSScript*s
Attachment #8782174 - Flags: review?(kvijayan) → review+
Pushed by shu@rfrn.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/121757de3a7d
Suppress sampling during compaction. (r=djvj)
https://hg.mozilla.org/mozilla-central/rev/121757de3a7d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.