Resizing iframe during MutationObserver callback hangs firefox reliably

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM
P3
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: James, Assigned: smaug)

Tracking

47 Branch
mozilla51
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [parity-chrome][parity-safari])

Attachments

(4 attachments, 1 obsolete attachment)

144 bytes, text/html
Details
864 bytes, text/html
Details
1.11 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Diff | Splinter Review
1.76 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

a year ago
Created attachment 8776910 [details]
test_iframe_stuff.html

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36

Steps to reproduce:

See attached files. Literally just open up test_iframe_stuff.html and make sure you have a users.js in your firefox profile with:

user_pref("capability.policy.localfilelinks.checkloaduri.enabled", "allAccess");
user_pref("capability.policy.policynames", "localfilelinks");

To allow easy loading of the local file (just for testing purposes, obviously in a real scenario you'd be loading it off a webserver).


Actual results:

Firefox just hangs and has to be force closed.


Expected results:

The page should load, and the iframe's height set to 500px.
(Reporter)

Comment 1

a year ago
Created attachment 8776911 [details]
insideiframe.html
(Reporter)

Updated

a year ago
Severity: normal → critical
OS: Unspecified → Windows 8.1
Priority: -- → P2
Hardware: Unspecified → x86_64
(Reporter)

Updated

a year ago
Priority: P2 → --
Attributes mutation notification about "style" attribute is sent infinitely.

Looks like, setting same style value sends the notification on Firefox and Edge, and doesn't send on Chrome and Safari.
Component: Untriaged → DOM
Product: Firefox → Core
Whiteboard: [parity-chrome][parity-safari]
(Assignee)

Comment 3

a year ago
In general setting attribute value to its old value is supposed to create a MutationRecord.
So, for consistency I'd say Gecko and Edge have the right behavior there

But I haven't yet found a spec which defines how changes to .style.* are supposed to be mapped to style attribute.

However, FF shouldn't hang, but slow script popup should allow one to stop the script.
(Reporter)

Updated

a year ago
Version: 49 Branch → 47 Branch
(Reporter)

Comment 4

a year ago
Workaround that was recommended (and that makes sense defensively) was to check the size before setting it to see if it's the same, if it is, don't set it to avoid infinite loop. This works for me for now, up to you guys to decide if this is a bug on the FF side or if it's just the way the api allows things and that it's up to the dev to code defensively. Thanks.
(Assignee)

Comment 5

a year ago
The bug here in Gecko is that it doesn't show the slow script popup.
Need to file webkit and chromium bugs about not generating the right MutationRecords, assuming some spec defines .style.* handling properly, and otherwise file some spec bugs.
(Reporter)

Comment 6

a year ago
Do I need to do that? Or should I leave that with you guys?
(Reporter)

Comment 7

a year ago
Also hang on, chromium? This stuff works fine in chrome, it's just firefox that has the issue.
(Assignee)

Comment 8

a year ago
It is a bug that chromium doesn't enter to endless loop.
But I can file that bug, after discussing blink folks.
(Reporter)

Comment 9

a year ago
Oh, I see. So essentially this api allowing an endless loop is a valid feature, and just something that should be taken into account by the dev, rather than being a bug. Correct?
(Assignee)

Comment 10

a year ago
Well at least element.setAttribute("foo", "bar"); element.setAttribute("foo", "bar");
is supposed to create two MutationRecords, and for consistency I think .style.* handling should too.
But I haven't yet found whether any specification deals with .style.* <-> style attribute mapping properly.
(Assignee)

Comment 11

a year ago
https://github.com/w3c/csswg-drafts/issues/370
Priority: -- → P3
(Assignee)

Updated

a year ago
Duplicate of this bug: 1290979
(Assignee)

Updated

a year ago
Depends on: 1291879
(Assignee)

Comment 13

a year ago
Created attachment 8777541 [details] [diff] [review]
patch

This is ├╝ber-hack which I would land only if Bug 1291879 doesn't get fixed soon.

It is just that effectively this same bug has been filed now at least twice within couple of days, so this should get fixed sooner than later.


The XPCJSRuntime::InterruptCallback stuff is to make that method less super slow in common cases.
Attachment #8777541 - Flags: review?(bobbyholley)
Comment on attachment 8777541 [details] [diff] [review]
patch

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

r=me on the XPConnect piece. I don't know enough about the context here of why the slow script dialog isn't working. Bill has been looking at the interrupt callback more recently, and probably has this more paged-in.

::: dom/base/nsDOMMutationObserver.cpp
@@ +909,5 @@
>      delete observers;
> +
> +    if (nestedCount % 100 == 0) {
> +      // In case we have nested mutation observer callbacks called, try to
> +      // ensure slow script dialog is triggered reasonable soon.

ensure the slow script dialog is reasonably soon. ("the" and "reasonably")
Attachment #8777541 - Flags: review?(wmccloskey)
Attachment #8777541 - Flags: review?(bobbyholley)
Attachment #8777541 - Flags: review+
I looked briefly at http://jsbin.com/gugacasuci/edit?html,output (referenced in the other bug). We seem to be ending a request pretty frequently during this code. Each time we end a request, the watchdog timer gets reset. There's not much the JS engine can do here.

Bobby mentioned to me that bug 1058695 might be related to this. That bug put a request around all promise activity. I think we just need to do the same thing for mutation observers. Ideally there would be one request around all the microtask stuff that we do. I don't think it would be too hard to do this, so I think we should avoid taking this hacky patch.

Olli, does that sound okay to you?
Flags: needinfo?(bugs)
(Assignee)

Comment 16

a year ago
(In reply to Bill McCloskey (:billm) from comment #15)
> I looked briefly at http://jsbin.com/gugacasuci/edit?html,output (referenced
> in the other bug). We seem to be ending a request pretty frequently during
> this code. Each time we end a request, the watchdog timer gets reset.
Why does request have such side effect?

> Bobby mentioned to me that bug 1058695 might be related to this. That bug
> put a request around all promise activity. I think we just need to do the
> same thing for mutation observers. Ideally there would be one request around
> all the microtask stuff that we do. I don't think it would be too hard to do
> this, so I think we should avoid taking this hacky patch.
> 
> Olli, does that sound okay to you?
That sounds good. Didn't know requests had such side effects. Let me test.
Flags: needinfo?(bugs)
(Assignee)

Comment 17

a year ago
Created attachment 8778201 [details] [diff] [review]
mutationobserverloop.diff
Assignee: nobody → bugs
Attachment #8777541 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8777541 - Flags: review?(wmccloskey)
Attachment #8778201 - Flags: review?(bobbyholley)
Comment on attachment 8778201 [details] [diff] [review]
mutationobserverloop.diff

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

::: dom/base/nsDOMMutationObserver.cpp
@@ +887,5 @@
>  
> +  // We need the AutoJSAPI to ensure the slow script dialog is triggered.
> +  // AutoJSAPI does that by pushing JSAutoRequest to stack.
> +  AutoJSAPI jsapi;
> +  jsapi.Init();

I would be more explicit and say that the AutoJSAPI needs to be on the _outside_ of the loop, rather than the inside, since it informs the JS engine that the processing of observers here is, from the user's perspective, one long-running operation that should be interrupted if it takes too long,
rather than a bunch of unrelated operations.

I think you should also put a call to JS_CheckForInterrupt inside the while loop like we do for promises:

http://searchfox.org/mozilla-central/source/dom/promise/Promise.cpp#1007

This means that the JS engine will notice an infinite loop even in the case that the microtasks are native (i.e. some DOM function via Function.bind) and don't ever trigger script.
Attachment #8778201 - Flags: review?(bobbyholley) → review+
(In reply to Olli Pettay [:smaug] from comment #16)
> > Olli, does that sound okay to you?
> That sounds good. Didn't know requests had such side effects. Let me test.

I don't think it's a side-effect anymore - it's the only remaining effect of JSAutoRequest. ;-)
(Assignee)

Comment 20

a year ago
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #18)
> I think you should also put a call to JS_CheckForInterrupt inside the while
> loop like we do for promises:
> 
> http://searchfox.org/mozilla-central/source/dom/promise/Promise.cpp#1007
> 
> This means that the JS engine will notice an infinite loop even in the case
> that the microtasks are native (i.e. some DOM function via Function.bind)
> and don't ever trigger script.
Hmm, is JS_CheckForInterrupt slow. I guess I should profile.



(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #19)
> (In reply to Olli Pettay [:smaug] from comment #16)
> > > Olli, does that sound okay to you?
> > That sounds good. Didn't know requests had such side effects. Let me test.
> 
> I don't think it's a side-effect anymore - it's the only remaining effect of
> JSAutoRequest. ;-)

Aha. Ok, we should rename it then.
(Assignee)

Comment 21

a year ago
JS_CheckForInterrupt(jsapi.cx()); crashes, since one needs to enter some compartment before that.

Ok, I guess one just must use the deprecated AutoSafeJSContext here to get cx easily and enter junk scope.
(In reply to Olli Pettay [:smaug] from comment #21)
> Ok, I guess one just must use the deprecated AutoSafeJSContext here to get
> cx easily and enter junk scope.

You can also just use JSAutoCompartment to enter the junk scope, which is prefereable. But there's really no reason why CheckForInterrupt should require being in a compartment - please file a bug on that if it's not easily fixable.
(Assignee)

Comment 23

a year ago
Created attachment 8778432 [details] [diff] [review]
mutationobserverloop_2.diff
(Assignee)

Comment 24

a year ago
FYI, we crash because of 
http://searchfox.org/mozilla-central/rev/389a3e0817b873a64376ec2b65f6807afbba9d8f/js/src/jit/Ion.cpp#2047

Using JSAutoCompartment and something else to get cx would be annoying. And need to ensure that
request is pushed to stack too. And manually dealing with junk scope ... not nice.
AutoSafeJSContext is really the cleanest solution here I can think of.
(In reply to Olli Pettay [:smaug] from comment #24)
> FYI, we crash because of 
> http://searchfox.org/mozilla-central/rev/
> 389a3e0817b873a64376ec2b65f6807afbba9d8f/js/src/jit/Ion.cpp#2047

Can we just fix that to also early-return in cx->compartment() is null?
 
> Using JSAutoCompartment and something else to get cx would be annoying.

I am suggesting:

// Hack: Need to be in some compartment to check for interrupt.
JSAutoCompartment ac(jsapi.cx(), xpc::UnprivilegedJunkScope());

> And
> need to ensure that
> request is pushed to stack too.

We already have the AutoJSAPI outside the while loop, no?

 And manually dealing with junk scope ... not
> nice.
> AutoSafeJSContext is really the cleanest solution here I can think of.

We really shouldn't add any new AutoSafeJSContext usage.

Comment 26

a year ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac120171cec
ensure slow script dialog is triggered when handling mutation observer callbacks, r=bholley
Flags: needinfo?(bugs)
I have a question about mutation observers and microtasks. Is it correct that we don't run mutation observers and promises together in a loop? Based on my reading of the spec, at a microtask checkpoint we're supposed to run all microtasks until there are no more left. What happens if we run a promise and that causes a mutation that should trigger a mutation observer. Currently it looks like we wait until the next turn of the event loop to run the observer. Shouldn't we run it in the same microtask checkpoint?

And if we do run them all together in a loop, then the JS request needs to include the whole loop.
(Assignee)

Comment 28

a year ago
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #25)

> // Hack: Need to be in some compartment to check for interrupt.
> JSAutoCompartment ac(jsapi.cx(), xpc::UnprivilegedJunkScope());
Well with AutoSafeJSContext you don't need any of that, nor AutoJSAPI, since if does all the automatically 


> 
> We really shouldn't add any new AutoSafeJSContext usage.
Well apparently we don't have any good replacement for this particular case.
I guess we should merge AutoSafeJSContext to AutoJSAPI, and have something like
InitWithJunkScope() or such.
(Assignee)

Comment 29

a year ago
(In reply to Bill McCloskey (:billm) from comment #27)
> I have a question about mutation observers and microtasks. Is it correct
> that we don't run mutation observers and promises together in a loop? 
Yes, but our Promises have never used microtasks but something similar.
MutationObserver is the thing for which microtasks were initially designed for.


>Based
> on my reading of the spec, at a microtask checkpoint we're supposed to run
> all microtasks until there are no more left. What happens if we run a
> promise and that causes a mutation that should trigger a mutation observer.
> Currently it looks like we wait until the next turn of the event loop to run
> the observer. Shouldn't we run it in the same microtask checkpoint?
Yes, Promises should have used microtasks. I haven't followed what has happened with them.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #28)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #25)
> 
> > // Hack: Need to be in some compartment to check for interrupt.
> > JSAutoCompartment ac(jsapi.cx(), xpc::UnprivilegedJunkScope());
> Well with AutoSafeJSContext you don't need any of that, nor AutoJSAPI, since
> if does all the automatically 

The semantics aren't the same though. With AutoSafeJSContext we stay in the Junk Compartment for all the executed code, which isn't great. If we need to hackily enter a compartment, it should be tightly scope. But per above I think we can just fix SM.

> > We really shouldn't add any new AutoSafeJSContext usage.
> Well apparently we don't have any good replacement for this particular case.

The default behavior of using the Junk Scope isn't something that we want people to be doing by default. That's why we shouldn't use it, and I'm asking for explicit syntax for doing the hacky thing.
I still would like to see comment 25 addressed.
Flags: needinfo?(bugs)
(Assignee)

Comment 32

a year ago
but yes, we should make Promises and MutationObserver to use the same stuff.
(Assignee)

Comment 33

a year ago
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #30)
> (In reply to Olli Pettay [:smaug] from comment #28)
> > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #25)
> > 
> > > // Hack: Need to be in some compartment to check for interrupt.
> > > JSAutoCompartment ac(jsapi.cx(), xpc::UnprivilegedJunkScope());
> > Well with AutoSafeJSContext you don't need any of that, nor AutoJSAPI, since
> > if does all the automatically 

Well, at least MutationObserver does now the same thing as what Promises do.
Flags: needinfo?(bugs)
(Assignee)

Comment 34

a year ago
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #31)
> I still would like to see comment 25 addressed.

So I wonder what the API should look like. Adding more explicit JSAPI usage to Gecko is always a bug ;)
(In reply to Olli Pettay [:smaug] from comment #34)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #31)
> > I still would like to see comment 25 addressed.
> 
> So I wonder what the API should look like. Adding more explicit JSAPI usage
> to Gecko is always a bug ;)

The code we're adding here is a hack, and so I don't think we should spend very much time trying to design an API for it. I just want the hack to be properly scoped, which is not the case with your patch?

Can you try just fixing Ion.cpp like I suggested in comment 25, which would remove the need for this hack? If that doesn't work for some reason, we need to scope the hack.
Flags: needinfo?(bugs)
(Assignee)

Comment 36

a year ago
That Ion stuff is called in InvokeInterruptCallback which uses compartment for rather valid stuff.
The fix doesn't look trivial, or I guess adding yet another nullcheck before
cx->compartment()->isDebuggee() is one option.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #36)
> That Ion stuff is called in InvokeInterruptCallback which uses compartment
> for rather valid stuff.
> The fix doesn't look trivial, or I guess adding yet another nullcheck before
> cx->compartment()->isDebuggee() is one option.

Whatever it uses the compartment for, we're not giving it any additional useful information by using the junk scope. So the best thing to do would be to just add null checks in the appropriate places so that this API can be used without being a compartment.

It's fine if you don't want to do that, but in that case please replace the AutoSafeJSContext with the AutoJSAPI (like in the patch in comment 17), and add a tightly-scoped { JSAutoCompartment } around the JS_CheckInterruptCallback call, with a comment explaining why we need to do that.
Flags: needinfo?(bugs)
(Assignee)

Comment 38

a year ago
I wish I understood why to make such 'inconsistent to what Promises do', and 'harder to understand' change, but I can land it tomorrow.
Flags: needinfo?(bugs)
(Assignee)

Updated

a year ago
Depends on: 1292699

Comment 39

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0ac120171cec
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.