Closed
Bug 1050360
Opened 10 years ago
Closed 10 years ago
Script blocker warning is triggering too much ("Unable to run script because scripts are blocked internally")
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: billm, Assigned: billm)
References
Details
Attachments
(2 files, 2 obsolete files)
10.07 KB,
text/plain
|
Details | |
4.71 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
Oops, I meant bug 1050358.
Adding Ehsan who knows editor.
Assignee | ||
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
(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.
Assignee | ||
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
For native anonymous content we will never "fix" it, because there really shouldn't be any reason to fix it.
Comment 9•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f8622ef4253
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1f8622ef4253
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•10 years ago
|
Summary: Script blocker warning is triggering too much → Script blocker warning is triggering too much ("Unable to run script because scripts are blocked internally")
Comment 15•10 years ago
|
||
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)
Comment 16•10 years ago
|
||
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 → ---
Comment 17•10 years ago
|
||
[Tracking Requested - why for this release]:
status-firefox34:
--- → affected
tracking-firefox34:
--- → ?
Comment 18•10 years ago
|
||
Could you please file a new bug. And do you perhaps have a testcase?
Comment 19•10 years ago
|
||
(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)
Comment 20•10 years ago
|
||
Liz - Is the issue that errors are thrown to the console? Does this impact the usability of Firefox?
Flags: needinfo?(lhenry)
Comment 21•10 years ago
|
||
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: 10 years ago → 10 years ago
Flags: needinfo?(lhenry)
Resolution: --- → FIXED
Updated•10 years ago
|
status-firefox34:
affected → ---
tracking-firefox34:
? → ---
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•