950745 - flag when we're processing urgent messages and disallow certain activities

Status: RESOLVED WONTFIX

(Core :: IPC, defect)

Description

[Benjamin Smedberg]

Urgent message semantics require that we can't enter nested event loops. In order to enforce this, we need a reliable flag to indicate that we're currently processing an urgent message (on the main thread, because that's the only thread which is targeted by this stuff). Then we need to disable the following things:

* any attempt to enter content JS should throw or crash (bholley said this should be possible on IRC a while back)
* DOM methods which are known to spin nested loops should throw: .alert() .confirm() .prompt() and synchronous XHR
* any attempt to nsIThread.processNextEvent should crash
* any entrance into plugins via NPAPI should crash
* DOM event dispatch should be delayed, fail, or crash (not sure exactly, but DOM events can enter content JS and we want to early-fail)

Comment 1

[Chris Peterson [:cpeterson]]

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: cpow-check

Is this kind of what you were thinking of? It's a pretty direct approach.

I didn't do any checks for ".alert() .confirm() .prompt() and synchronous XHR" because I figured those will pretty much always run processNextEvent. Maybe I'm wrong though. If so we can fix them.

I also wasn't sure where to put checks into NPAPI. I was considering a bunch of places in nsJSNPRuntime.cpp, but I figured it might be easier to just ask you.

Comment 3

[Chris Peterson [:cpeterson]]

Can any of these code paths be called from addons? Addon developers probably won't test with Firefox debug builds. Can we use release build assertions? (I'm trying to add a MOZ_NIGHTLY_ASSERT macro for Nightly-only testing in a different bug.)

Comment 4

[Benjamin Smedberg]

Comment on attachment 8412274 [details] [diff] [review]
cpow-check

These should be MOZ_RELEASE_ASSERT.

It's unfortunate to have the AutoProcessUrgentMessage stuff in each method implementation: since the MessageChannel already has mDispatchingUrgentMessageCount, please instead attach the detection at http://hg.mozilla.org/mozilla-central/annotate/09a19b25b9cf/ipc/glue/MessageChannel.cpp#l1093 and put ProcessingUrgentMessages in ipc/glue.

We should also add the assert at the beginning of nsWindow::WindowProc.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: cpow-check v2

This patch uses MOZ_RELEASE_ASSERT, but only in Nightly. I measured the cost of these assertions, and we get about 1% slower on the Octane benchmark. So I don't think it's a good idea to turn this on everywhere. Also, CPOWs are disabled outside nightly. I'll have to think some more about how to address this long-term.

Does this look okay otherwise? I still am wondering what to do about NPAPI.

Comment 6

[Benjamin Smedberg]

Comment on attachment 8413989 [details] [diff] [review]
cpow-check v2

>diff --git a/ipc/glue/MessageChannel.cpp b/ipc/glue/MessageChannel.cpp

>+    JS_ASSERT(NS_IsMainThread());

Use MOZ_ASSERT in ipc/glue.

>diff --git a/js/src/vm/Interpreter.cpp b/js/src/vm/Interpreter.cpp
>--- a/js/src/vm/Interpreter.cpp
>+++ b/js/src/vm/Interpreter.cpp
>@@ -383,16 +383,21 @@ ExecuteState::pushInterpreterFrame(JSCon
>                                                               type_, evalInFrame_);
> }
> 
> bool
> js::RunScript(JSContext *cx, RunState &state)
> {
>     JS_CHECK_RECURSION(cx, return false);
> 
>+#ifdef NIGHTLY_BUILD
>+    if (ScriptEntryHook hook = cx->runtime()->scriptEntryHook_)
>+        (*hook)(cx, state.script());
>+#endif

Is this the performance-expensive part? If so I'm fine with making this nightly-only...

>diff --git a/widget/windows/nsWindow.cpp b/widget/windows/nsWindow.cpp
>--- a/widget/windows/nsWindow.cpp
>+++ b/widget/windows/nsWindow.cpp
>@@ -4299,16 +4299,20 @@ inline static mozilla::HangMonitor::Acti
>   return mozilla::HangMonitor::kActivityUIAVail;
> }
> 
> // The WndProc procedure for all nsWindows in this toolkit. This merely catches
> // exceptions and passes the real work to WindowProcInternal. See bug 587406
> // and http://msdn.microsoft.com/en-us/library/ms633573%28VS.85%29.aspx
> LRESULT CALLBACK nsWindow::WindowProc(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam)
> {
>+#ifdef NIGHTLY_BUILD
>+  MOZ_RELEASE_ASSERT(!ipc::ProcessingUrgentMessages());
>+#endif

I'm fine with making this nightly-only for now since that's where we have CPOWs, but I don't think this and the nsThread changes would noticeably affect perf and I'd like to have them in all builds with CPOWs.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: rebased

I needed this for bug 1034321 so here is a rebased version.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: script-entry

This patch splits out the JS changes. The basic idea is to check, in the child, that if a CPOW message comes in from the parent then we don't run any content scripts while we're answering it.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: cpow-checks

The remainder of the patch. I made the thread and WindowProc changes apply to more than just nightly as you asked.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Oh, and yes, the JS changes are the expensive part. I suspect the event dispatching stuff is pretty performance-sensitive too.

Comment 11

[Luke Wagner [:luke]]

I understand this is a best-effort assert, but it seems like you'd get better coverage if you put this assert in enterCompartment.  That would pick up all the corner cases where we don't go through RunScript.  Would that work?

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Luke Wagner [:luke] from comment #11)
> I understand this is a best-effort assert, but it seems like you'd get
> better coverage if you put this assert in enterCompartment.  That would pick
> up all the corner cases where we don't go through RunScript.  Would that
> work?

That's not really the property we want. We want CPOWs to be able to touch content objects--as in get properties off them and such. We just want to avoid running JS code, such as if the add-on decided to bypass Xrays and call some content code.

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 8454690 [details] [diff] [review]
script-entry

Ok, that makes sense.  Could we give the hook a less general-sounding name, indicating it is only used for assertions in nightly?  Like, say, AssertOnScriptEntryHook or something?  Also, do you suppose we could put the jsfriendapi.h declaration in #ifdef?  Just to make it really clear :)

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Grant]

Is it possible that this change broke Adblock plus? I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1041334 and the only change that happened on that build was this?

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I backed out part of this until we can get bug 1042587 landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a3c113c0da7

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

My backout wasn't sufficient to make the assertions go away (we still hit the JS assertion after I disabled the event handling one), so I backed out everything.

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

I'll reland when bug 1042587 lands.

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64226fe74be1

Comment 20

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/64226fe74be1

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I had to back out these event assertions again. They're triggering too much. The main cases are PermitUnload and in add-ons that directly call dispatchEvent via a CPOW.

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

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 8454691 [details] [diff] [review]
cpow-checks

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

::: widget/windows/nsWindow.cpp
@@ +4340,5 @@
>  // exceptions and passes the real work to WindowProcInternal. See bug 587406
>  // and http://msdn.microsoft.com/en-us/library/ms633573%28VS.85%29.aspx
>  LRESULT CALLBACK nsWindow::WindowProc(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam)
>  {
> +  MOZ_RELEASE_ASSERT(!ipc::ProcessingUrgentMessages());

I think it'd be safe to move this to MessageChannel::NotifyGeckoEventDispatch() -

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsAppShell.cpp#257
http://mxr.mozilla.org/mozilla-central/source/ipc/glue/WindowsMessageLoop.cpp#711

Curious, are cpows limited to a main thread channel?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Jan de Mooij [:jandem]]

Bill, do you think it's worth keeping the AssertOnScriptEntryHook code (in SM)? I just noticed it's unused, but if we're planning on using it soon maybe we should keep it?

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

No, we can remove it.

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

Bill, do we need to do anything with this now that CPOWs are out of our browser code? Does this matter when it comes to a11y messages?

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This has been superseded by the cancellation code.