1353559 - Exited browser actors might be asked for their form

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Exited browser actors can still be asked for their form, presumably if someone is actively list tabs during the time they exit.

We should update the `form` code path to guard against `this._browser` being null.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1353559 - Guard against null browser in exited actor.

Review commit: https://reviewboard.mozilla.org/r/126574/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126574/

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc1fa583488771cdc6e80005b33db0801a1cc2f9

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8854630 [details]
Bug 1353559 - Guard against null browser in exited actor.

https://reviewboard.mozilla.org/r/126574/#review129336

Did you had exception in tests or in the product?
It looks like we should stop using actors as soon as they are exited,
so I suspect there is some more fixes to do... Especially if that happens in the product!

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1353559 - Filter out exited browsers from tab list.

This adds higher level protection to ignore exited browser actors, so we don't
end up triggering methods like `form` on them, since they won't make much sense
anyway.

Review commit: https://reviewboard.mozilla.org/r/126798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126798/

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8854630 [details]
Bug 1353559 - Guard against null browser in exited actor.

https://reviewboard.mozilla.org/r/126574/#review129336

I noticed this while running some net monitor tests.  I haven't seen it in product yet.

The problem happens during onListTabs.  Here is the sequence I saw:

1. The browser's actor is alive
2. Someone calls listTabs
3. `onListTabs` asks for the list of browser actors, which is async with various promises, etc.
4. The browser's actor exits
5. `onListTabs` gets the list back and starts iterating, and the list contains the exited actor

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8854842 [details]
Bug 1353559 - Filter out exited browsers from tab list.

https://reviewboard.mozilla.org/r/126798/#review129416

Thanks a lot for this another cleanup, I think this is the kind of tweaks very valuable for our tests!

I'm wondering again if there is still something to fix in the netmonitor test you were running to make it wait correctly for listTabs before proceeding? It sounds like this listTabs request could very likely finish after the test ends and overlap the next one?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f74c1423f855
Guard against null browser in exited actor. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/29a30ad34f35
Filter out exited browsers from tab list. r=ochameau

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8854842 [details]
Bug 1353559 - Filter out exited browsers from tab list.

https://reviewboard.mozilla.org/r/126798/#review129416

That's a good point, I'll check into this!

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/f74c1423f855
https://hg.mozilla.org/mozilla-central/rev/29a30ad34f35

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot [:ochameau] from comment #6)
> Comment on attachment 8854842 [details]
> Bug 1353559 - Filter out exited browsers from tab list.
> 
> https://reviewboard.mozilla.org/r/126798/#review129416
> 
> Thanks a lot for this another cleanup, I think this is the kind of tweaks
> very valuable for our tests!
> 
> I'm wondering again if there is still something to fix in the netmonitor
> test you were running to make it wait correctly for listTabs before
> proceeding? It sounds like this listTabs request could very likely finish
> after the test ends and overlap the next one?

Looks like the issue was a performance front getter being triggered on toolbox close.  I'll address it in bug 1353897.