Closed Bug 1456274 Opened 6 years ago Closed 6 years ago

Try to remove client-side progress listeners from target

Categories

(DevTools :: Framework, enhancement, P3)

enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

(Whiteboard: dt-fission)

Attachments

(3 files)

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.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
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)
(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 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?
Attachment #8970732 - Flags: review?(poirot.alex) → review+
Comment on attachment 8970733 [details]
Bug 1456274 - Rewrite browser_target_events in async style.

https://reviewboard.mozilla.org/r/239472/#review245252
Attachment #8970733 - Flags: review?(poirot.alex) → review+
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)
Attachment #8970734 - Flags: review?(poirot.alex) → review+
(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 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 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`.
Pushed by jryans@gmail.com:
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
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
Flags: needinfo?(jryans)
Flags: needinfo?(jryans)
Pushed by jryans@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/2c3e334c75b6
https://hg.mozilla.org/mozilla-central/rev/08f354110a57
https://hg.mozilla.org/mozilla-central/rev/6c411b0781cc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
Whiteboard: dt-fission
You need to log in before you can comment on or make changes to this bug.