1352157 - DevTools - preserve lazy browser's laziness whenever possible

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Description

[u462496]

Some actions in devtools will cause all lazy browsers (bug 906076) to be inserted.

When devtools > memory is activated, a frame script is injected into every browser.  Currently this action will force all lazy browsers to be inserted into the document removing them from their lazy state.

Closing the devtools pane also causes the same result (which begs the question, why are we injecting frame scripts because we're closing devtools?).  It actually triggers the same code (resource://devtools/server/main.js:1027:9).

Besides losing the optimization advantage, a major downside to letting lazy browsers be inserted in a situation like this is that if there is a high volume of tabs in the lazy state, the user will experience unexpected choking while they are all being inserted.

Comment 1

[Dão Gottwald [:dao]]

(In reply to Kevin Jones from comment #0)
> Some actions in devtools will cause all lazy browsers (bug 906076) to be
> inserted.
> 
> When devtools > memory is activated, a frame script is injected into every
> browser.

Why doesn't this use a global frame script?

If there's a good reason for loading this frame script per browser, then yes, this should probably skip uninitialized browsers. Might also need to take care of browsers that get initialized while the memory tool is running (or maybe not, I'm not really familiar with this tool).

Comment 2

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

Hoping to investigate this soon.

Comment 3

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

Looks like this is a quirk of how DevTools code paths have evolved over time.  

There's a `listTabs` API that will walk over every tab, and to talk to it, we load a frame script.  (In general, we should move productions callers away from `listTabs` so we can be more confident that lazy tabs stay lazy.)  This is not meant to be the "main" flow for DevTools, as in most cases, we only interact with one tab at a time, so I think it's fine to load frame scripts into a tab at a time like we do now.

For the case of the memory tool, it doesn't even care about the tabs at all. :( This all just accidental API cruft, essentially.  So, I'll clean that up.

The part about the frame script being loaded when _closing_ DevTools is quite bad, I agree.  That should now be fixed by bug 1353897.

Comment 4

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

Attachment: Bug 1352157 - Improve root actor style with async / await.

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

Comment 5

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

Attachment: Bug 1352157 - Avoid listTabs for global actors.

Adds a new `getRoot` request to the root actor which lists the global actors
only (leaving out the tabs).  This is a much better fit for callers who want to
access some global actor only, since it avoids visiting every tab, which could
be a very expensive operation.

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

Comment 6

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

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

Comment 7

[u462496]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> Looks like this is a quirk of how DevTools code paths have evolved over
> time.  
> 
> There's a `listTabs` API that will walk over every tab, and to talk to it,
> we load a frame script.  (In general, we should move productions callers
> away from `listTabs` so we can be more confident that lazy tabs stay lazy.) 
> This is not meant to be the "main" flow for DevTools, as in most cases, we
> only interact with one tab at a time, so I think it's fine to load frame
> scripts into a tab at a time like we do now.
> 
> For the case of the memory tool, it doesn't even care about the tabs at all.
> :( This all just accidental API cruft, essentially.  So, I'll clean that up.
> 
> The part about the frame script being loaded when _closing_ DevTools is
> quite bad, I agree.  That should now be fixed by bug 1353897.

That sounds great, Ryan, thanks for working on this.

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8855621 [details]
Bug 1352157 - Improve root actor style with async / await.

https://reviewboard.mozilla.org/r/127478/#review130258

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8855622 [details]
Bug 1352157 - Avoid listTabs for global actors.

https://reviewboard.mozilla.org/r/127480/#review130264

I'm wondering if we should remove all listTabs usages?
I can easily see us doing it in many other places...
At first sight it looks like it is mostly used in tons of tests (I imagine it may start being an issue once browser become lazy).
Otherwise the only one production codepath I see is in webide and about:debugging.
The one in about:debugging may also be an issue.

::: devtools/server/actors/root.js:335
(Diff revision 1)
> -    };
> +    });
>  
> -    /* If a root window is accessible, include its URL. */
> +    // If a root window is accessible, include its URL.
>      if (this.url) {
>        reply.url = this.url;
>      }

This looks like something for getRoot form,
but at the same time, this seems to be a dead piece of code. I can't find where `url` is defined...

Comment 10

[Alexandre Poirot [:ochameau]]

By the way, thanks for all the fixes you made everywhere these last days!
I imagine you may easily have improved devtools perfs with all of them.

Comment 11

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

Comment on attachment 8855622 [details]
Bug 1352157 - Avoid listTabs for global actors.

https://reviewboard.mozilla.org/r/127480/#review130264

Yes, we should try to move away entirely if we can.  I filed bug 1354596 about it.  For WebIDE / about:debugging, it's less clear, since they _do_ want to list all tabs, but maybe there's something more minimal we can offer that is parent process only (without loading a frame script into each tab).

> This looks like something for getRoot form,
> but at the same time, this seems to be a dead piece of code. I can't find where `url` is defined...

I agree it appears to be dead code, looking at the listTabs reply, it's not included.  I'll remove it.

Comment 12

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

Comment on attachment 8855622 [details]
Bug 1352157 - Avoid listTabs for global actors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127480/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5fd82d57a5f2
Improve root actor style with async / await. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/d017da7f66ba
Avoid listTabs for global actors. r=ochameau

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5fd82d57a5f2
https://hg.mozilla.org/mozilla-central/rev/d017da7f66ba