Closed Bug 950745 Opened 6 years ago Closed 4 years ago

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

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX
mozilla34
Tracking Status
e10s + ---

People

(Reporter: benjamin, Assigned: billm)

References

Details

Attachments

(2 files, 3 obsolete files)

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)
No longer blocks: e10s-it1
Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".
tracking-e10s: --- → +
Assignee: nobody → mrbkap
Attached patch cpow-check (obsolete) — Splinter Review
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.
Assignee: mrbkap → wmccloskey
Status: NEW → ASSIGNED
Attachment #8412274 - Flags: feedback?(benjamin)
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 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.
Attachment #8412274 - Flags: feedback?(benjamin)
Attached patch cpow-check v2 (obsolete) — Splinter Review
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.
Attachment #8412274 - Attachment is obsolete: true
Attachment #8413989 - Flags: feedback?(benjamin)
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.
Attachment #8413989 - Flags: feedback?(benjamin) → feedback+
Attached patch rebased (obsolete) — Splinter Review
I needed this for bug 1034321 so here is a rebased version.
Attached patch script-entrySplinter Review
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.
Attachment #8413989 - Attachment is obsolete: true
Attachment #8453158 - Attachment is obsolete: true
Attachment #8454690 - Flags: review?(luke)
Attached patch cpow-checksSplinter Review
The remainder of the patch. I made the thread and WindowProc changes apply to more than just nightly as you asked.
Attachment #8454691 - Flags: review?(benjamin)
Oh, and yes, the JS changes are the expensive part. I suspect the event dispatching stuff is pretty performance-sensitive too.
Attachment #8454691 - Flags: review?(benjamin) → review+
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?
(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 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 :)
Attachment #8454690 - Flags: review?(luke) → review+
Depends on: 1040726
https://hg.mozilla.org/mozilla-central/rev/ed1736c6367a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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?
Depends on: 1041370
Depends on: 1042587
Depends on: 1042653
Depends on: 1043709
I backed out part of this until we can get bug 1042587 landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a3c113c0da7
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1045102
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.
https://hg.mozilla.org/mozilla-central/rev/64226fe74be1
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla33 → mozilla34
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
https://hg.mozilla.org/mozilla-central/rev/d368bd458f99
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
Flags: needinfo?(wmccloskey)
No, we can remove it.
Flags: needinfo?(wmccloskey)
Depends on: 1168150
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?
Flags: needinfo?(wmccloskey)
This has been superseded by the cancellation code.
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Flags: needinfo?(wmccloskey)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.