1034084 - Reflow actor shouldn't emit reflow events after the window has been closed

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

Because the actor relies on a timer to batch events together, we may hit a race condition whereby a reflow event is sent after the window has been closed.
This is particularly frequent when running tests.

Typically, at the end of an inspector mochitest-browser test, you might see this exception:

 0:07.63 console.error:
 0:07.63   Message: Error: Got an invalid root window in DocumentWalker
 0:07.63   Stack:
 0:07.64     DocumentWalker@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:2779:1
 0:07.64 documentWalker@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:2761:10
 0:07.64 WalkerActor<.parentNode@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:1116:18
 0:07.64 exports.NodeActor<.form@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:213:22
 0:07.64 types.addActorType/type<.write@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:275:1
 0:07.64 types.addArrayType/<.write@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:182:25
 0:07.64 Arg<.write@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:411:12
 0:07.64 Request<.write/str<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:582:1
 0:07.64 Request<.write@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:580:15
 0:07.64 Actor<._sendEvent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:833:18
 0:07.64 Actor<.initialize/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:821:11
 0:07.64 emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:96:9
 0:07.64 WalkerActor<._onReflows@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:1002:7
 0:07.64 EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/event-emitter.js:136:11
 0:07.64 LayoutChangesObserver.prototype<._startEventLoop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/layout.js:281:7
 0:07.64 notify@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/timers.js:40:9

It bubbles right up to the NodeActor because the inspector listens for reflows to determine which nodes have had their visibility change.

Comment 1

[Patrick Brosset <:pbro>]

The solution is to test for the existence of the tabActor's window in the batching loop.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: bug1034084-avoid-reflow-events-after-window-close v1.patch

This suppresses the intermittent stacktrace I've been seeing when running some of the inspector tests.

Comment 3

[Patrick Brosset <:pbro>]

Pending try build: https://tbpl.mozilla.org/?tree=Try&rev=5626958be16b

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8450195 [details] [diff] [review]
bug1034084-avoid-reflow-events-after-window-close v1.patch

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

Ah, this should fix the exceptions at the end of test runs.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: bug1034084-avoid-reflow-events-after-window-close v2.patch

An xpcshell was failing with my previous patch. That's because in that test, I was mocking the TabActor and since the layout actor now relies on tabActor.attached, I needed to mock that up too.

So this new patch does just that.

New pending try: https://tbpl.mozilla.org/?tree=Try&rev=fe90cf6fd44a

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/3271ce25db00

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3271ce25db00