nsContentUtils::RemoveScriptBlocker should release-assert that it's on the main thread

RESOLVED FIXED in Firefox 40



4 years ago
4 years ago


(Reporter: jld, Assigned: jld)



Firefox Tracking Flags

(firefox40 fixed)



(1 attachment, 1 obsolete attachment)

nsContentUtils::RemoveScriptBlocker can run a variety of interesting main-thread-only code, including arbitrary content JS; this probably justifies a release assertion that it's called on the main thread.

We could also do the check in AddScriptBlocker; I'm not sure how incrementally useful this is.  If I have to pick one I'd rather have the assertion in RemoveScriptBlocker, because that's the one that runs the runnables.
Created attachment 8584909 [details] [diff] [review]

The patch is trivial, but the performance implications might not be.
Attachment #8584909 - Flags: review?(bugs)
Could you do some microbenchmarking here? Something silly like
data:text/html,<script>setInterval(function () {  var s = (new Date()).getTime();  for (var i = 0; i < 500000; ++i ) document.documentElement.setAttribute("foo", i);  document.body.textContent = ((new Date()).getTime() - s) + "ms" }, 50); </script>
and look at if RemoveScriptBlocker shows up worse in the profile than it shows up currently?
Comment on attachment 8584909 [details] [diff] [review]

I'm somewhat reluctant to add more release asserts because it hints that
patches aren't being tested enough using debug builds and in that case 
better fix would be to increase testing on debug builds.

But r+, if you don't see the assert badly in opt build profiles.
Attachment #8584909 - Flags: review?(bugs) → review+
I added some RDTSCs to RemoveScriptBlocker, and tried the test case in comment #3 on OS X natively and in a Linux VM (VMware Fusion).  I don't have a Windows VM at the moment, but if Windows implements TLS significantly differently then it might be worth gathering data there as well.

The assertion costs about 18 cycles (6.4ns); the rest of RemoveScriptBlocker is about 280 (100ns) natively and 180 (64ns) in the VM, and I don't know why that difference exists.

The loop calls RemoveScriptBlocker 1000000 times (2x per iteration) and runs in about 500-520 ms natively and 660-690 ms in the VM.

So this is a significant increase relative to RemoveScriptBlocker itself, but it's <1.5% of the setAttribute microbenchmark.  I'm not sure how we feel about that.
Thoughts about comment #5?
Flags: needinfo?(bugs)
Anything above 0.5% or so sounds rather high to me.
(we have the issue in general that we have tons of "doesn't-show-too-badly-in-profiles" stuff, which makes optimizations rather hard.)

Perhaps release assert only in nightlies and aurora?
Flags: needinfo?(bugs)
Created attachment 8589925 [details] [diff] [review]
Patch [v2; less assertive]

MOZ_DIAGNOSTIC_ASSERT is MOZ_RELEASE_ASSERT on nightly/aurora and MOZ_ASSERT on beta/release, so that works.
Attachment #8584909 - Attachment is obsolete: true
Attachment #8589925 - Flags: review+
Jed, is this ready to land?
(In reply to Andrew McCreight [:mccr8] from comment #11)
> Jed, is this ready to land?

Looks like it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c14f9f9660bb

Thanks for the reminder.
Keywords: checkin-needed
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.