Closed Bug 1050360 Opened 5 years ago Closed 5 years ago

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

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached file 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.
Attached patch silence-warning (obsolete) — Splinter Review
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.
Attachment #8469379 - Flags: review?(bent.mozilla)
Depends on: 1050358
Attached patch silence-warning v2 (obsolete) — Splinter Review
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.
Attachment #8469379 - Attachment is obsolete: true
Attachment #8469379 - Flags: review?(bent.mozilla)
Attachment #8469402 - Flags: review?(bent.mozilla)
Needed some changes for this to work in opt builds.
Attachment #8469402 - Attachment is obsolete: true
Attachment #8469402 - Flags: review?(bent.mozilla)
Attachment #8470405 - Flags: review?(bent.mozilla)
(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 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.
Attachment #8470405 - Flags: review?(bent.mozilla) → review?(bugs)
For native anonymous content we will never "fix" it, because there really shouldn't be any reason to
fix it.
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.
Attachment #8470405 - Flags: review?(bugs) → review+
Duplicate of this bug: 1050358
Duplicate of this bug: 1055802
No longer depends on: 1050358
https://hg.mozilla.org/mozilla-central/rev/1f8622ef4253
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Summary: Script blocker warning is triggering too much → Script blocker warning is triggering too much ("Unable to run script because scripts are blocked internally")
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.
Flags: needinfo?(florin.mezei)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
[Tracking Requested - why for this release]:
Could you please file a new bug.
And do you perhaps have a testcase?
(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.
Flags: needinfo?(florin.mezei)
Liz - Is the issue that errors are thrown to the console? Does this impact the usability of Firefox?
Flags: needinfo?(lhenry)
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.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Flags: needinfo?(lhenry)
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.