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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

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.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Patch

The patch is trivial, but the performance implications might not be.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8584909 [details] [diff] [review]
Patch

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.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

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.

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Thoughts about comment #5?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Patch [v2; less assertive]

MOZ_DIAGNOSTIC_ASSERT is MOZ_RELEASE_ASSERT on nightly/aurora and MOZ_ASSERT on beta/release, so that works.

Comment 11

[Andrew McCreight [:mccr8]]

Jed, is this ready to land?

Comment 12

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(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.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4dcce22831f

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4dcce22831f