827990 - inspector tests use DOM MutationObserver API incorrectly

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Nathan Froyd [:froydnj]]

I'm debugging some issues with page event queues (bug 715376) and am running into an error with browser_inspector_markup_edit.js.  The below log is from Linux64, but the error occurs on all platforms:

https://tbpl.mozilla.org/php/getParsedLog.php?id=18584189&tree=Try#error1

As near as I can tell, what happens is this:

1. We start a test from inside a function called from the event loop (on a clean tree, the function is injected directly into the event loop; in the above tree, the function is called via setTimeout)

The call to executeSoon with this handler function is here:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/test/browser_inspector_markup_edit.js#353

which is going to trigger the handler for a new-node event here:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/test/browser_inspector_markup_edit.js#347

which is going to call the functions below.

2. We select a node of interest:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/test/browser_inspector_markup_edit.js#248

This code causes a MutationObserver to be set up on the node in question.

3. Perform various things to trigger the mutation observer:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/test/browser_inspector_markup_edit.js#258

4. And finally, check various things, some of which should have been set by the mutation observer:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/test/browser_inspector_markup_edit.js#269

Notice that this is all done in the context of the handler function.  Which means--assuming I understand smaug's explanation of the MutationObserver API--that the changes we expect to see in step 4 don't actually happen until the handler stops running.

At least, that's what seems to be happening in my tree.  I don't understand why this same issue doesn't show up in an unpatched tree.

The fix is simple enough.  But I wanted to file this bug to see if somebody had an opinion on my analysis.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: fix browser_inspector_markup_edit.js so mutation observer events propagate correctly

This is the easy fix.  Note that this matches what we already do in the
undoRedo() function a few lines later.

Comment 2

[Paul Rouget [:paul]]

See also modifications of this test in bug 814889 (to land soon), which, more or less, do the same thing but for another part of the test.

Comment 3

[Nathan Froyd [:froydnj]]

Ah, thanks for pointing that out!  If that's going to land soon (and it looks like it is), it's a much better solution.

Comment 4

[Paul Rouget [:paul]]

(In reply to Nathan Froyd (:froydnj) from comment #3)
> Ah, thanks for pointing that out!  If that's going to land soon (and it
> looks like it is), it's a much better solution.
> 
> *** This bug has been marked as a duplicate of bug 814889 ***

Does 814889's patch covers what is addressed in your patch?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Paul Rouget [:paul] from comment #4)
> (In reply to Nathan Froyd (:froydnj) from comment #3)
> > Ah, thanks for pointing that out!  If that's going to land soon (and it
> > looks like it is), it's a much better solution.
> > 
> > *** This bug has been marked as a duplicate of bug 814889 ***
> 
> Does 814889's patch covers what is addressed in your patch?

Whoops!  No, they don't.

Comment 6

[Paul Rouget [:paul]]

I'll do the same modifications for testAsyncSetup.

Comment 7

[Paul Rouget [:paul]]

Attachment: v1

Nathan, can you tell me if this works for you?

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 699634 [details] [diff] [review]
v1

Yup, that seems to fix the bug for me!  Thanks!

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/19bbc804d676

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/19bbc804d676