1456274 - Try to remove client-side progress listeners from target

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P3)

Description

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

The target currently includes client-side progress listeners that would appear to replicate similar tracking done server-side.

We should attempt to remove this from the target to reduce complexity and force local and remote connections to use the same code.

Comment 1

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

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

Comment 2

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

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

Comment 3

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

Attachment: Bug 1456274 - Remove local progress listener in DevTools target.

DevTools targets have used a local progress listener for a long time to track
tab navigation.  However, this is redundant with server side code that does the
same thing.

Removing this from the target reduces differences between local and remote
debugging.  It also simplifies one piece of the target, which is a massively
twisted module.

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

Comment 4

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

Attachment: Bug 1456274 - Rewrite browser_target_events in async style.

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

Comment 5

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

Attachment: Bug 1456274 - Add target.makeRemote in several tests.

Navigation events now require a "remoted" target.  Add `makeRemote` calls to a
tests which make use of these features.

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

Comment 6

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

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

Comment 7

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

Added Talos runs as well:

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

Comment 8

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

There are a few changes on DAMP, but it's hard for me to tell if there are meaningful...

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=532752fb414268dbbb31289d73441a2683e77570&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1&selectedTimeRange=172800

damp console.objectexpand                       2.92% 14.06 (high)
damp console.openwithcache.open.settle.DAMP   -11.93% 22.45 (high)
damp custom.webconsole.close.DAMP              10.96%  4.09 (med)
damp simple.saveHeapSnapshot                   11.46%  5.71 (high)

Comment 9

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)

> damp console.objectexpand                       2.92% 14.06 (high)
> damp console.openwithcache.open.settle.DAMP   -11.93% 22.45 (high)
> damp custom.webconsole.close.DAMP              10.96%  4.09 (med)
> damp simple.saveHeapSnapshot                   11.46%  5.71 (high)

It looks like a very random set of changes which typically comes from comparing against last x days of m-c. These tests baseline must have changed recently.
You may push your base revision to get better results.

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8970732 [details]
Bug 1456274 - Remove local progress listener in DevTools target.

https://reviewboard.mozilla.org/r/239470/#review245250

Thanks for cleaning these things up!

::: devtools/client/framework/target.js:499
(Diff revision 1)
>    _setupListeners: function() {
> -    this._webProgressListener = new TabWebProgressListener(this);
> -    this.tab.linkedBrowser.addProgressListener(this._webProgressListener);
>      this.tab.addEventListener("TabClose", this);
>      this.tab.parentNode.addEventListener("TabSelect", this);
>      this.tab.ownerDocument.defaultView.addEventListener("unload", this);

Does that still work !?
Aren't we throwing when using CPOW now?
Are you planning to follow-up to remove even more listeners? I'm not sure unload, nor TabClose are necessary here.
Or getting rid of non-remote codepath entirely as I think it is only used in tests, right?

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8970733 [details]
Bug 1456274 - Rewrite browser_target_events in async style.

https://reviewboard.mozilla.org/r/239472/#review245252

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 8970734 [details]
Bug 1456274 - Add target.makeRemote in several tests.

https://reviewboard.mozilla.org/r/239474/#review245254

Looks good but that brings some questions:
- was that the few tests that was still using non remote target codepath? or just the one depending on navigation events?
- could we somehow force makeRemote call at (test) framework level? I imagine that's bug 1072764 (also see bug 1397020)

Comment 13

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

(In reply to Alexandre Poirot [:ochameau] from comment #9)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> 
> > damp console.objectexpand                       2.92% 14.06 (high)
> > damp console.openwithcache.open.settle.DAMP   -11.93% 22.45 (high)
> > damp custom.webconsole.close.DAMP              10.96%  4.09 (med)
> > damp simple.saveHeapSnapshot                   11.46%  5.71 (high)
> 
> It looks like a very random set of changes which typically comes from
> comparing against last x days of m-c. These tests baseline must have changed
> recently.
> You may push your base revision to get better results.

You are right.  Here's a comparison with the specific base rev I used:

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&originalRevision=39ccabfd7d0712a45335325cb24b0e0b2ba498c7&newProject=try&newRevision=532752fb414268dbbb31289d73441a2683e77570&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1

No important changes found.

Comment 14

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

Comment on attachment 8970732 [details]
Bug 1456274 - Remove local progress listener in DevTools target.

https://reviewboard.mozilla.org/r/239470/#review245250

> Does that still work !?
> Aren't we throwing when using CPOW now?
> Are you planning to follow-up to remove even more listeners? I'm not sure unload, nor TabClose are necessary here.
> Or getting rid of non-remote codepath entirely as I think it is only used in tests, right?

This one is `tab.ownerDocument`, so it means the browser window.  Maybe you thought it said `contentDocument`?

In any case, removing these remaining ones feels less straightforward to me...  Perhaps some are important during tab / window shutdown where we may lose the server side abruptly?  Where possible though, I agree we should remove this local stuff if we can do it on there server side without losing features.

Comment 15

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

Comment on attachment 8970734 [details]
Bug 1456274 - Add target.makeRemote in several tests.

https://reviewboard.mozilla.org/r/239474/#review245254

There are many tests that don't call `makeRemote` themselves.  Typically they open a toolbox and show some tool whose panel module will eventually call `makeRemote`.

The only things that's change here is for tests that explicitly wanted the navigation events.  If they are needed, someone in the pipeline now needs to call `makeRemote`.  I think it's okay to just tune these few tests for now.  We should solve the "real" problem via the general bugs you linked about `makeRemote`.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cf34fe2036e
Remove local progress listener in DevTools target. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/97e8882d0343
Rewrite browser_target_events in async style. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/707d5e397407
Add target.makeRemote in several tests. r=ochameau

Comment 17

[Dorel Luca [:dluca]]

Backed out 3 changesets (bug 1456274) for Linting failure. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=175610877&repo=autoland&lineNumber=252

Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-04-25T22:16:37.449Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/framework/target.js:11:9 | 'XPCOMUtils' is assigned a value but never used. (no-unused-vars)
[taskcluster 2018-04-25 22:16:37.741Z] === Task Finished ===
[taskcluster 2018-04-25 22:16:37.741Z] Unsuccessful task run with exit code: 1 completed in 546.262 seconds

Push with fails:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=707d5e397407bdee7cdfbb5ee6ff644126b778a9&selectedJob=175610877

Backout:
https://hg.mozilla.org/integration/autoland/rev/e43c1c2ce4131918fffab239dc7b61453ce2c736

Comment 18

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

Comment on attachment 8970732 [details]
Bug 1456274 - Remove local progress listener in DevTools target.

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

Comment 19

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

Comment on attachment 8970733 [details]
Bug 1456274 - Rewrite browser_target_events in async style.

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

Comment 20

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

Comment on attachment 8970734 [details]
Bug 1456274 - Add target.makeRemote in several tests.

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c3e334c75b6
Remove local progress listener in DevTools target. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/08f354110a57
Rewrite browser_target_events in async style. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/6c411b0781cc
Add target.makeRemote in several tests. r=ochameau

Comment 22

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/2c3e334c75b6
https://hg.mozilla.org/mozilla-central/rev/08f354110a57
https://hg.mozilla.org/mozilla-central/rev/6c411b0781cc