Bug 16606 - MutationObserver doesn't provide a safe way to suspend & resume observation
MutationObserver doesn't provide a safe way to suspend & resume observation
Status: RESOLVED FIXED
Product: WebAppsWG
Classification: Unclassified
Component: DOM
unspecified
PC All
: P2 normal
: ---
Assigned To: Anne
public-webapps-bugzilla
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-03 00:22 UTC by Rafael Weinstein
Modified: 2012-04-05 08:49 UTC (History)
8 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Rafael Weinstein 2012-04-03 00:22:26 UTC
It's a common use to want to have the mutation callback not report any changes it makes in the following invocation. This is trivially accomplished by calling disconnect() at the beginning of the callback and observe() immediately before exiting.

disconnect() clears any queue records, but this is safe inside a mutation callback because the observer has just been delivered all mutations and is guaranteed that no other actor could have made new ones.

However, if the observer would like to ignore changes which are made outside the mutation callback (say in response to a XHR callback), suspending observation by calling disconnect/observe risks losing mutation records already queued.

[This came up via a user of the MutationSummary library whose use case is synchronizing a JSON structure with DOM. The problem arises when trying to ignore changes made by applying remote JSON changes to the DOM]

It seems to me there are two approaches to this:

1) Add an explicit suspect/resume API to MutationObserver which does what you might guess.
2) Add some sort of pullMutations() call which would allow code to synchronously fetch any queued records, thus making it safe to use disconnect/observe.

Editorial:

1) Has the benefit of being a fairly clear API given the use and might also allow the user agent to better optimize usage because what's needed is actually being expressed. However (as Adamk noted), it has the down side that suspend() may be confused with disconnect() and cause the DOM to be doing alot of unneeded work
2) Is less direct in its form, but it addresses this use case, and also another use which we've suspected we'd eventually need: the desire to "pull" mutations because you'd like to synchronize your state one an imperative API call.

My $.02: I prefer adding something like pullMutations() because I think we'll need it for other reasons, and it kills two birds with one stone. Also, as much as I like the directness of suspend/resume, I'm worried about pages calling suspend and never calling resume.

Opinions?
Comment 1 Rafael Weinstein 2012-04-03 00:27:57 UTC
Sorry: 

Option (1) should read: ..."Add an explicit suspend/resume API"
Comment 2 Olli Pettay 2012-04-03 00:44:35 UTC
suspend/resume feels quite awkward.
pullMutations() is simpler, though I don't quite like the name.
Maybe takeMutations() ?
Comment 3 Rafael Weinstein 2012-04-03 01:01:26 UTC
I don't have a strong opinion about naming. Just off the top of my head, here are some other options:

-clear
-retrieve
-empty
-deliver
-fetch
Comment 4 Ojan Vafai 2012-04-03 01:23:24 UTC
I'm OK with #2. Could we also have disconnect return the list of records it's
clearing? That way, users of the API are more likely to realize that they might
be losing records if they ignore the return value.
Comment 5 Olli Pettay 2012-04-03 01:24:30 UTC
disconnect() will possibly return the mutation observer itself.
There is another bug open for that.
Comment 6 Olli Pettay 2012-04-03 01:25:31 UTC
Depends on what should happen when method is called.

deliver() sounds good, if the idea is that the mutation observer callback
is called and the current mutation records passed as parameters.
Comment 7 Ryosuke Niwa 2012-04-03 01:36:38 UTC
I agree with that option 2 (adding deliver()) will be better than option 1.

On the other hand, having disconnect() return the list of mutation records it cleared would satisfy this particular use case ("ignor[ing] changes made by applying remote JSON changes to the DOM") without adding new API.

Can we just do that (having disconnect() return the list) and avoid adding new API for now?
Comment 8 Olli Pettay 2012-04-03 01:54:01 UTC
It feels odd if disconnect() returns the records.
Is there any reason to not add deliver() now ?
Comment 9 Ryosuke Niwa 2012-04-03 02:26:23 UTC
(In reply to comment #8)
> It feels odd if disconnect() returns the records.
> Is there any reason to not add deliver() now ?

I'm not certain the specific behavior of deliver() you've described (i.e. it calls the mutation observer) would work. e.g. what happens if the script calls deliver() inside a mutation observer?
Comment 10 Olli Pettay 2012-04-03 07:33:34 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > It feels odd if disconnect() returns the records.
> > Is there any reason to not add deliver() now ?
> 
> I'm not certain the specific behavior of deliver() you've described (i.e. it
> calls the mutation observer) would work. e.g. what happens if the script calls
> deliver() inside a mutation observer?
Inside MutationObserver? You mean MutationObserver's callback?
Well, if there are new records, the callback would be called again, if not,
nothing would happen.
Comment 11 Ryosuke Niwa 2012-04-03 08:13:29 UTC
(In reply to comment #10)
> Inside MutationObserver? You mean MutationObserver's callback?
> Well, if there are new records, the callback would be called again, if not,
> nothing would happen.

Right, and I don't think that's a desirable behavior (called again) because now mutation observer callback can be called mutually recursively (with deliver()) without starting an event loop.

i.e. I prefer takeMutations() over deliver().
Comment 12 Olli Pettay 2012-04-03 08:19:50 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > Inside MutationObserver? You mean MutationObserver's callback?
> > Well, if there are new records, the callback would be called again, if not,
> > nothing would happen.
> 
> Right, and I don't think that's a desirable behavior (called again) because now
> mutation observer callback can be called mutually recursively (with deliver())
> without starting an event loop.

Why is that not desirable behavior? And what has mutation observer callback
to do with event loop?


> 
> i.e. I prefer takeMutations() over deliver().
takeMutations() or some such which just returns the current records and
clears the record list is ok to me.
I don't see much difference between takeMutations() and deliver().
Comment 13 Ryosuke Niwa 2012-04-03 08:35:45 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > Right, and I don't think that's a desirable behavior (called again) because now
> > mutation observer callback can be called mutually recursively (with deliver())
> > without starting an event loop.
> 
> Why is that not desirable behavior?

Because writing a re-entrant callback is hard. This is one big issue with the existing mutation events. If you make any DOM mutations inside a mutation event handler, then you might get called synchronously, and it's really hard to reason about.

> And what has mutation observer callback to do with event loop?

I thought a mutation observer callback can be called recursively if you start a new event loop by e.g. showModalDialog inside the callback itself, and observable DOM mutations are made within that event loop. Am I missing / misunderstanding something?
Comment 14 Olli Pettay 2012-04-03 08:39:09 UTC
(In reply to comment #13)
> 
> Because writing a re-entrant callback is hard. This is one big issue with the
> existing mutation events. If you make any DOM mutations inside a mutation event
> handler, then you might get called synchronously, and it's really hard to
> reason about.
> 
> > And what has mutation observer callback to do with event loop?
> 
> I thought a mutation observer callback can be called recursively if you start a
> new event loop by e.g. showModalDialog inside the callback itself, and
> observable DOM mutations are made within that event loop. Am I missing /
> misunderstanding something?

Yes, callbacks can be called recursively. But how would deliver() make that
situation any worse?

But sounds like you don't like deliver() too much, and I don't
care whether it is deliver() or takeMutations(), so perhaps we should do the
latter ?
Comment 15 Rafael Weinstein 2012-04-03 10:42:05 UTC
I'm not worried about reentrancy (exactly) because if you ended up being reentrant, you did it to yourself (i.e. it'd be pretty insane to call deliver inside of a delivery callback. However, if you didn't want your callback invoked, then you have to jump through some hoops to save the records away somewhere. Basically, this might lead to some awkward code patterns.

So, yeah, let's go with something that simply returns the records directly and doesn't invoke the callback.
Comment 16 Rafael Weinstein 2012-04-04 18:56:45 UTC
From IRC discussion (rafael, olli & anne): takeRecords().