1220012 - Leak nsPipe from GeckoDriver.mozBrowserClose

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P2)

Description

[Ting-Yu Chou [:ting] (away)]

Two nsPipe are created for the input/ouput stream in MarionetteServer.onSocketAccepted(). After MarionetteServer.onConnectionClosed() gets called, the connection (Dispatcher) is still referenced by leaked GeckoDriver.mozBrowserClose.

Now mozBrowserClose is removed when enter GeckoDriver.receiveMessage(), but probably it should be cleared before running any command.

Comment 1

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #0)
> Now mozBrowserClose is removed when enter GeckoDriver.receiveMessage(), but
> probably it should be cleared before running any command.

Hmm, it should be all right to clear the mozBrowserClose in receiveMessage() as it will receive the command's respone/error message from listener. Am double checking.

Comment 2

[Andreas Tolfsen ❲:ato❳]

(In reply to Ting-Yu Chou [:ting] from comment #1)
> Hmm, it should be all right to clear the mozBrowserClose in receiveMessage()
> as it will receive the command's respone/error message from listener. Am
> double checking.

Note that receiveMessage is only called when the listener sends a message back to the driver; not at any other time.

Comment 3

[Ting-Yu Chou [:ting] (away)]

Listener does send message (Marionette:ok,error,done) back to the driver, just driver does not listen to it (bug 1107706 part 5 made the change).

I'd like to move the code removing mozBrowserClose from GeckoDriver.receiveMessage() to GeckoDriver.responseCompleted(). How do you think?

Comment 4

[Ting-Yu Chou [:ting] (away)]

Maybe we should simply add the messages back, so driver will receive the them. GeckoDriver.responseCompleted() is not always called, e.g., when it's out of sync.

Comment 5

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #4)
> Maybe we should simply add the messages back, so driver will receive the
> them. GeckoDriver.responseCompleted() is not always called, e.g., when it's
> out of sync.

Correction: Maybe we should simply add the message listeners back, so driver will receive the messages.

Comment 6

[Ting-Yu Chou [:ting] (away)]

Attachment: patch v1

Is now testing the patch on device, will check on Monday.

Comment 7

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8681119 [details] [diff] [review]
patch v1

The leak is not observed after running ~1.5h MTBF with the patch.

Comment 8

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #7)
> Comment on attachment 8681119 [details] [diff] [review]
> patch v1
> 
> The leak is not observed after running ~1.5h MTBF with the patch.

It's monday morning, and still don't see the leaks.

Comment 9

[Bobby Chien]

David and Andreas, please help to review patch in comment 7. Many thanks.

Comment 10

[Andreas Tolfsen ❲:ato❳]

Sorry for my late response to this bug, but I've been travelling to Japan.

(In reply to Ting-Yu Chou [:ting] from comment #0)
> Two nsPipe are created for the input/ouput stream in
> MarionetteServer.onSocketAccepted(). After
> MarionetteServer.onConnectionClosed() gets called, the connection
> (Dispatcher) is still referenced by leaked GeckoDriver.mozBrowserClose.
> 
> Now mozBrowserClose is removed when enter GeckoDriver.receiveMessage(), but
> probably it should be cleared before running any command.

Can you please help me understand what reference prevents the nsPipe from being cleaned up?

I get that Dispatcher gets an instance of DebuggerTransport, which in turn encapsulates the input and output streams.  However, the Dispatcher isn't passed to GeckoDriver.

From what I can tell, MarionetteServer#onConnectionClosed attempts to clean up the Dispatcher by deleting its reference to it form MarionetteServer#conns:

    delete this.conns[id];

Is what you're saying that as long as GeckoDriver keeps an active event listener, mozbrowserclose in this case, it will never be cleaned up?  But how does GeckoDriver reference Dispatcher?

If we want to clean up the GeckoDriver#mozBrowserClose event listener when we close the connection (MarionetteServer#onConnectionClosed) I feel removing it in GeckoDriver#receiveMessage is the wrong solution.  Wouldn't this be better suited for a shutdown/cleanup method in GeckoDriver that we call when we want to deconstruct Dispatcher from MarionetteServer?

Comment 11

[Ting-Yu Chou [:ting] (away)]

(In reply to Andreas Tolfsen (:ato) from comment #10)
> Is what you're saying that as long as GeckoDriver keeps an active event
> listener, mozbrowserclose in this case, it will never be cleaned up?  But
> how does GeckoDriver reference Dispatcher?

GeckoDriver.emulator.send references to Dispatcher.send, which keeps the Dispatcher instance alive and Dispatcher.conn points to the DebuggerTransport. Also GeckoDriver is leaked to window through mozbrowserclose event listener.

> 
> If we want to clean up the GeckoDriver#mozBrowserClose event listener when
> we close the connection (MarionetteServer#onConnectionClosed) I feel
> removing it in GeckoDriver#receiveMessage is the wrong solution.  Wouldn't
> this be better suited for a shutdown/cleanup method in GeckoDriver that we
> call when we want to deconstruct Dispatcher from MarionetteServer?

In that way, the listener could be added many times but remove just once. Note the listener is added for commands actionChain, multiAction, clickElement, and singleTap, that's why I remove it in GeckoDriver.receiveMessage when the command is done.

Comment 12

[Andreas Tolfsen ❲:ato❳]

(In reply to Ting-Yu Chou [:ting] from comment #11)
> In that way, the listener could be added many times but remove just once.
> Note the listener is added for commands actionChain, multiAction,
> clickElement, and singleTap, that's why I remove it in
> GeckoDriver.receiveMessage when the command is done.

I've gone over this three times, but I still don't get it.  Why do we need to remove mozbrowserclose only when ok, done, or error comes back and not otherwise?

Comment 13

[Ting-Yu Chou [:ting] (away)]

(In reply to Andreas Tolfsen (:ato) from comment #12)
> (In reply to Ting-Yu Chou [:ting] from comment #11)
> > In that way, the listener could be added many times but remove just once.
> > Note the listener is added for commands actionChain, multiAction,
> > clickElement, and singleTap, that's why I remove it in
> > GeckoDriver.receiveMessage when the command is done.
> 
> I've gone over this three times, but I still don't get it.  Why do we need
> to remove mozbrowserclose only when ok, done, or error comes back and not
> otherwise?

Because from what I can see, ok, done, or error denotes the command is finished. And the listener is added before doing the commands, so remove it when the command gets done. It's also OK if you think we should just roll back to the code before refactoring, which removes the listener whenever receives a message.

Comment 14

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8681119 [details] [diff] [review]
patch v1

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

(In reply to Ting-Yu Chou [:ting] from comment #13)
> Because from what I can see, ok, done, or error denotes the command is
> finished. And the listener is added before doing the commands, so remove it
> when the command gets done.

OK, thanks for all the explanations.

This patch changes the mozBrowserClose check from being run on every message from the listener to only on ok, done, and error.  Provided this is fine, I see no reason to integrate this patch as it is.

Comment 15

[Ting-Yu Chou [:ting] (away)]

Thanks for reviewing. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c1462d242e0

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee70b9acae8a

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/ee70b9acae8a

Comment 18

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8681119 [details] [diff] [review]
patch v1

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1107706
User impact if declined: memory leaks from using marionette
Testing completed: marionette unit tests on try
Risk to taking this patch (and alternatives if risky): low as it just removes listeners
String or UUID changes made by this patch: none

Comment 19

[Bobby Chien]

Mahe, please help to approve this uplift request. Thanks.

Comment 20

[Mahendra Potharaju [:mahe]]

Comment on attachment 8681119 [details] [diff] [review]
patch v1

Approved for 2.5 uplifts. 

Thanks

Comment 21

[Carsten Book [:Tomcat]]

landed as http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/8d98a5a0ad7e