1291257 - Resizing iframe during MutationObserver callback hangs firefox reliably

Status: RESOLVED FIXED

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

Description

[James]

Attachment: 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.

Comment 1

[James]

Attachment: insideiframe.html

Comment 2

[Tooru Fujisawa [:arai]]

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.

Comment 3

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

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.

Comment 4

[James]

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.

Comment 5

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

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.

Comment 6

[James]

Do I need to do that? Or should I leave that with you guys?

Comment 7

[James]

Also hang on, chromium? This stuff works fine in chrome, it's just firefox that has the issue.

Comment 8

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

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

Comment 9

[James]

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?

Comment 10

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

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.

Comment 11

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

https://github.com/w3c/csswg-drafts/issues/370

Comment 13

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

Attachment: 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.

Comment 14

[Bobby Holley (:bholley)]

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")

Comment 15

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

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?

Comment 16

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

(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.

Comment 17

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

Attachment: mutationobserverloop.diff

Comment 18

[Bobby Holley (:bholley)]

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.

Comment 19

[Bobby Holley (:bholley)]

(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. ;-)

Comment 20

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

(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.

Comment 21

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

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.

Comment 22

[Bobby Holley (:bholley)]

(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.

Comment 23

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

Attachment: mutationobserverloop_2.diff

Comment 24

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

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.

Comment 25

[Bobby Holley (:bholley)]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac120171cec
ensure slow script dialog is triggered when handling mutation observer callbacks, r=bholley

Comment 27

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

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.

Comment 28

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

(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.

Comment 29

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

(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.

Comment 30

[Bobby Holley (:bholley)]

(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.

Comment 31

[Bobby Holley (:bholley)]

I still would like to see comment 25 addressed.

Comment 32

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

but yes, we should make Promises and MutationObserver to use the same stuff.

Comment 33

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

(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.

Comment 34

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

(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 ;)

Comment 35

[Bobby Holley (:bholley)]

(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.

Comment 36

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

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.

Comment 37

[Bobby Holley (:bholley)]

(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.

Comment 38

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

I wish I understood why to make such 'inconsistent to what Promises do', and 'harder to understand' change, but I can land it tomorrow.

Comment 39

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0ac120171cec