Closed Bug 1384513 Opened 2 years ago Closed 2 years ago

Enable passing either JS::AutoCheckCannotGC or JS::AutoSuppressGCAnalysis to functions that currently only take the former


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox56 --- wontfix
firefox57 --- fixed


(Reporter: till, Assigned: jonco, NeedInfo)




(2 files, 1 obsolete file)

As discussed on IRC yesterday, this changes the signatures of public functions taking JS::AutoCheckCannotGC to instead take a common sub-class of that and  JS::AutoSuppressGCAnalysis.
Attachment #8890290 - Flags: review?(jcoppeard)
Comment on attachment 8890290 [details] [diff] [review]
Change all public APIs to take JS::AutoRequireNoGC instead of JS::AutoCheckCannotGC

Review of attachment 8890290 [details] [diff] [review]:

Thanks for doing this.

One problem that I didn't spot earlier: the zero-argument constructor of AutoAssertNoAlloc does nothing unless you later call disallowAlloc(), whereas if you construct AutoAssertNoGC with no arguments it gets the current context from TLS and assumes it to be valid.  The use of AutoSuppressGCAnalysis in HelperThread::threadLoop() happens when there is no valid context.  This behaviour of AutoAssertNoAlloc was surprising and arguably wrong, but that's not the point here.

To preserve the existing behaviour, can you make the bodies of AutoAssertNoGC's constructor and destructor conditional on cx_ being non-null?
Attachment #8890290 - Flags: review?(jcoppeard) → review+
Pushed by
Change all public APIs to take JS::AutoRequireNoGC instead of JS::AutoCheckCannotGC. r=jonco
Pushed by
Follow-up to fix bustage from template usage of functions with a changed signature, on a CLOSED TREE. r=bustage
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
NI to make sure we don't forget about the regressions.
Flags: needinfo?(jcoppeard)
I'll back this out this afternoon. I can work around the issue that I created the patch for, so it doesn't seem useful to keep the regression in.
Flags: needinfo?(till)
Pushed by
Backed out changeset 78859e22ce17 for perf regressions.
Backed out changeset 000f28217a30 for perf regressions.
backout followup - Add AutoCheckCannotGC in Stream.cpp. r=me
This caused a regression in at least the sunspider format-xparb test.

This is probably caused by the fact that AutoAssertNoGC is compiled in release builds, because it uses MOZ_DIAGNOSTIC_ASSERT not MOZ_ASSERT.
Assignee: till → jcoppeard
This is the original patch with just the changes to make API functions take AutoRequireNoGC instead of AutoCheckCannotGC.
Attachment #8890290 - Attachment is obsolete: true
Attachment #8903229 - Flags: review+
This patch removes AutoAssertNoAlloc which was only used by AutoSuppressGCAnalysis and updates the latter to use AutoAssertNoGC in debug builds, or to have no assertions in release builds.

The problem with the original patch was that AutoAssertNoGC is a diagnostic mode assert and so is compiled in nightly debug builds.  This caused performance regressions.
Attachment #8903262 - Flags: review?(sphink)
Attachment #8903262 - Flags: review?(sphink) → review+
Pushed by
Change all public APIs to take JS::AutoRequireNoGC instead of JS::AutoCheckCannotGC. r=jonco
Remove AutoAssertNoAlloc r=sfink
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1396156
Too late for 56. Mass won't fix for 56.
You need to log in before you can comment on or make changes to this bug.