1042587 - [e10s] Need a script blocker when dispatching urgent messages

Status: VERIFIED FIXED

(Core :: IPC, defect)

Description

[Alice0775 White]

Steps To Reproduce:
1. Make sure e10s, set browser.tabs.remote.autostart = true and restart
2. Install Adblock Plus development build
   https://adblockplus.org/devbuilds/adblockplus/00latest.xpi
3. Make sure, EasyList is enabled from "Filter preferences…"(Ctrl+Shift+F)
4. Open a certain web page http://explosm.net/comics/3630/

Actual Results:
Tab crashed

Expected Results:
Tab should not crash.
bp-a0f0bfd9-91d8-425e-9394-a3acf2140723


Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/274a3f27b497
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140717203733
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed1736c6367a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140717211032
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=274a3f27b497&tochange=ed1736c6367a

Regressed by:
ed1736c6367a	Bill McCloskey — Bug 950745 - Flag when we're processing urgent messages and disallow certain activities (r=bsmedberg,luke)

Comment 1

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

Attachment: script-blocker

Great testcase. Thanks Alice.

I remember now that one reason I was waiting to land 950745 was that I needed to put these script blockers in place. Anyway, here they are.

It would be nice to be able to isolate this code within js/ipc, but I don't think that's possible. We can't actually unblock event handlers until we've sent the reply to the CPOW message, and that happens in the IPC code. So I had to stick the script blockers there.

There's also an issue where we have to block scripts whenever sending sync messages. I tried to explain why in the comment. The thing I'm most worried about here is that we're changing interrupt messages, which affects plugins. Do you think this will be a problem Benjamin? Note that we can still run JS code if we call it directly through JSAPI--this is what the CPOW code does. The script blocker only prevents us from running JS code "by accident"--through event handlers, DOM mutation events, mutation observers, and stuff like that.

If this is a problem for plugins, I'm not entirely sure how to fix it. One way would be to have a way to distinguish plugin channels from other kinds and avoid the script blockers on the plugin channels. I can't think of anything else.

Comment 2

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

Comment on attachment 8462256 [details] [diff] [review]
script-blocker

The try run for this isn't looking good.

Comment 3

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

https://tbpl.mozilla.org/?tree=Try&rev=bfd599047555

From the try run, it looks like we do need to be able to run scripts when sending interrupt messages for plugins. Benjamin, what do you think I should do here? I can add a special case for plugins, but it seems a little ugly. I'm having trouble thinking of alternatives though. An interrupt call that's not in the plugin protocol (i.e., the CreateWindow message) can receive urgent messages while it's waiting for a reply, and we want to block scripts in that case until we've received the CreateWindow reply.

Comment 4

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

Attachment: script-blocker v2

I figured this out. We don't need the script blocker in front of InterruptCall because interrupt messages are designed for reentry, so it's okay to run any pending scripts inside an InterruptCall call.

Comment 5

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

Attachment: warn-script-ignored

Talked to bent about this and he thought it would be good to find all the places where we ignore scripts because of script blockers and generate error console warnings in those cases. I did an audit and found a few. Some of them are weird corner cases: clipboard stuff, setUserData handlers, and DOM mutation events (now deprecated I think). The one in nsPresShell seems like a generic backstop. Hopefully callers should ensure we don't reach there.

Comment 6

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

Attachment: script-blocker v3

bent had concerns that:
1. We're running the script blocker code even when we're not on the main thread, which is bad.
2. The interrupt thing I did seems sketchy.

I decided maybe the simplest approach is for each protocol to opt into script blocking. If they do, then we can always block, even for intr messages. And we don't need to worry about threads, since the only people who ask to block scripts will be on the main thread.

https://tbpl.mozilla.org/?tree=Try&rev=bfa55667432d

Comment 7

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

Attachment: svg-fix

Found this while doing the audit. Got an r+ over irc from roc.

Comment 8

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

Attachment: script-blocker v4

Arghh. I forgot that I had already tried the approach of preventing events from running during InterruptCall but only for PContent. It doesn't work for the following reason:

- child sends intr message
- while waiting for the intr reply, the child decides to dispatch some queued async messages from the parent
- when dispatching one of those async messages, we need to run an event handler

However, this makes it clear that it should be perfectly fine to run events while waiting for an interrupt reply. So I think my previous solution of allowing scripts to run in InterruptCall should be fine. I wrote a longer comment in the patch explaining why.

Comment 9

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

Here's the try push for the most recent patch:
https://tbpl.mozilla.org/?tree=Try&rev=742273224326

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8465129 [details] [diff] [review]
warn-script-ignored

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

::: content/base/public/nsContentUtils.h
@@ +1521,5 @@
>    }
>  
>    /**
> +   * Call this function if !IsSafeToRunScript() and we fail to run the script
> +   * (rather than using AddScriptRunner as we usually do).

Nit: Add that document is optional, only used for printing the URI to the console.

::: content/base/src/nsContentUtils.cpp
@@ +5098,5 @@
> +  nsAutoString msg;
> +  if (aDocument) {
> +    nsCOMPtr<nsIURI> uri = aDocument->GetDocumentURI();
> +    if (uri) {
> +      nsAutoCString spec;

Nit: Just nsCString here.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8465658 [details] [diff] [review]
script-blocker v4

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

::: content/base/src/nsContentUtils.cpp
@@ +5043,3 @@
>    if (!sScriptBlockerCount) {
>      NS_ASSERTION(sRunnersCountAtFirstBlocker == 0,
>                   "Should not already have a count");

Nit: Might as well make this MOZ_ASSERT too

::: ipc/glue/MessageChannel.cpp
@@ +188,5 @@
>  };
>  
> +namespace {
> +
> +class MOZ_STACK_CLASS ScriptBlocker {

Nit: Call this "MaybeScriptBlocker"? Something that indicates it doesn't always block scripts.

@@ +195,5 @@
> +                  MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
> +        : mBlocked(aChannel->ShouldBlockScripts())
> +    {
> +        MOZ_GUARD_OBJECT_NOTIFIER_INIT;
> +       if (mBlocked) {

Nit: Indent is funny here somehow.

::: ipc/glue/MessageChannel.h
@@ +90,5 @@
>      }
>  
> +    void BlockScripts()
> +    {
> +        mBlockScripts = true;

I'd be happier if this MOZ_ASSERT'd that we're on the main thread here.

Comment 13

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0b34b80257ee
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/248dee37875a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9ad8af59efe3

Comment 14

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/0b34b80257ee
https://hg.mozilla.org/mozilla-central/rev/248dee37875a
https://hg.mozilla.org/mozilla-central/rev/9ad8af59efe3

Comment 15

[Bogdan Maris, Desktop QA]

Reproduced the initial issue on old Nightly (2014-07-21), verified that the issue is fixed on latest Aurora 34.0a2.