1050360 - Script blocker warning is triggering too much ("Unable to run script because scripts are blocked internally")

Status: RESOLVED FIXED

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

Description

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

Attachment: stack

In bug 1042587 I added a warning whenever we fail to run some JS code because a script blocker is in place. This seems to be triggering a lot in nightly, even with e10s disabled. All the cases I've seen happen when we remove a DOM node in the middle of some editor code. In that case, we're potentially failing to dispatch a DOM mutation event. I'm attaching a stack.

Comment 1

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

Attachment: silence-warning

It seems to me that we're just not doing a good job of dispatching these events. The reason we added the warning in bug 1042587 was to make sure that adding a script blocker doesn't cause us to fail to dispatch events. But given that we don't do a very good job here, maybe we should just silence the warning in this case. We're getting this warning so often that people will just ignore it. Ideally we would fix the mutation event code here, but that doesn't seem like a good investment of our time.

Comment 2

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

Attachment: silence-warning v2

Smaug points out in bug 1050360 that we don't fire the assertion that was here before for anonymous content. I just adapted that assertion for this warning. It still seems broken to me, but I guess this is better than not warning at all.

Comment 3

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

Oops, I meant bug 1050358.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Adding Ehsan who knows editor.

Comment 5

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

Attachment: silence-warning v3

Needed some changes for this to work in opt builds.

Comment 6

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

(In reply to Bill McCloskey (:billm) from comment #2)
> Created attachment 8469402 [details] [diff] [review]
> silence-warning v2
> 
> Smaug points out in bug 1050360 that we don't fire the assertion that was
> here before for anonymous content. I just adapted that assertion for this
> warning. It still seems broken to me, but I guess this is better than not
> warning at all.

Why is that broken? _native_ anonymous content is rather special, and we don't fire mutation events for it anyway.

Comment 7

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

Comment on attachment 8470405 [details] [diff] [review]
silence-warning v3

> Why is that broken? _native_ anonymous content is rather special, and we
> don't fire mutation events for it anyway.

Usually with an assertion you can argue "I know this will never fire because X", and the purpose of the assertion is to ensure that X remains true even if somebody refactors the code or something. With this code, it seems like the approach is more "let's see if this ever fires and fix it if it does". I don't understand this code well, though, so maybe I'm wrong.

Comment 8

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

For native anonymous content we will never "fix" it, because there really shouldn't be any reason to
fix it.

Comment 9

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

Comment on attachment 8470405 [details] [diff] [review]
silence-warning v3


>+    if (!(aChild->IsNodeOfType(nsINode::eCONTENT) &&
>+         static_cast<nsIContent*>(aChild)->
>+         IsInNativeAnonymousSubtree()) &&
These days we have also faster, non-virtual way to do this.
!(aChild->IsContent() && aChild->AsContent()->IsInNativeAnonymousSubtree())



>+        !sDOMNodeRemovedSuppressCount)
>+    {
{ should be in the previous line.

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f8622ef4253

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1f8622ef4253

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

This showed up as an issue in testing for Firefox 34.0b7 last night. Should we reopen this, or file a new bug? Florin, do you have any information to add here?

I'm seeing this from the notes on manual testing,

"[Known: Bug 1050360] "Unable to run script because scripts are blocked internally"  JS error thrown several times while browsing facebook, youtube, google plus" for Mac OS X 10.8.5.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Other comments from exploratory/manual testing,
"Unable to run script because scripts are blocked internally" JS error thrown several times while browsing google plus.  This should be fixed on Firefox 34 according to https://bugzil.la/1050360, but the expected result here - following this patch - is not clear."

I'm going to go ahead and reopen this.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

[Tracking Requested - why for this release]:

Comment 18

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

Could you please file a new bug.
And do you perhaps have a testcase?

Comment 19

[Vasilica Tamas, QA [:vtamas]]

(In reply to Olli Pettay [:smaug] from comment #18)
> Could you please file a new bug.
> And do you perhaps have a testcase?

Filed Bug 1096357 for this.

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Liz - Is the issue that errors are thrown to the console? Does this impact the usability of Firefox?

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Lawrence, actually it looks like this is another console-only error. I'll close it now that there's a new bug, and it doesn't need tracking.