Closed Bug 1042587 Opened 6 years ago Closed 5 years ago

[e10s] Need a script blocker when dispatching urgent messages

Categories

(Core :: IPC, defect, critical)

33 Branch
x86_64
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla34
Tracking Status
e10s + ---
firefox34 --- verified

People

(Reporter: alice0775, Assigned: billm)

References

Details

(Keywords: crash)

Crash Data

Attachments

(3 files, 3 obsolete files)

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)
See Also: → 1041656
Attached patch script-blocker (obsolete) — Splinter Review
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.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8462256 - Flags: review?(benjamin)
Comment on attachment 8462256 [details] [diff] [review]
script-blocker

The try run for this isn't looking good.
Attachment #8462256 - Flags: review?(benjamin)
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.
Flags: needinfo?(benjamin)
Attached patch script-blocker v2 (obsolete) — Splinter Review
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.
Attachment #8462256 - Attachment is obsolete: true
Attachment #8462759 - Flags: review?(benjamin)
Flags: needinfo?(benjamin)
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.
Attachment #8462759 - Attachment is obsolete: true
Attachment #8462759 - Flags: review?(benjamin)
Attachment #8465129 - Flags: review?(bent.mozilla)
Attached patch script-blocker v3 (obsolete) — Splinter Review
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
Attachment #8465135 - Flags: review?(bent.mozilla)
Attached patch svg-fixSplinter Review
Found this while doing the audit. Got an r+ over irc from roc.
Attachment #8465137 - Flags: review+
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.
Attachment #8465135 - Attachment is obsolete: true
Attachment #8465135 - Flags: review?(bent.mozilla)
Attachment #8465658 - Flags: review?(bent.mozilla)
Duplicate of this bug: 1045933
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.
Attachment #8465129 - Flags: review?(bent.mozilla) → review+
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.
Attachment #8465658 - Flags: review?(bent.mozilla) → review+
Summary: [e10s] Tab crashes on certain page with Adblock Plus if browser.tabs.remote.autostart = true → [e10s] Need a script blocker when dispatching urgent messages
Depends on: 1050358
Flags: qe-verify+
Reproduced the initial issue on old Nightly (2014-07-21), verified that the issue is fixed on latest Aurora 34.0a2.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.