Closed
Bug 1301487
Opened 8 years ago
Closed 8 years ago
Update new debugger frontend (9/8/2016)
Categories
(DevTools :: Debugger, defect)
DevTools
Debugger
Tracking
(firefox51 fixed)
RESOLVED
FIXED
Firefox 51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: jlong, Assigned: jlong)
References
Details
Attachments
(2 files, 3 obsolete files)
3.33 MB,
patch
|
Details | Diff | Splinter Review | |
610 bytes,
patch
|
Details | Diff | Splinter Review |
Need to push an update today which will use the local CodeMirror and React modules, and also try to land the first batch of tests.
Assignee | ||
Comment 1•8 years ago
|
||
If you're looking for a patch Brian, here it is. We need to merge a few more PRs before actually committing this, but these mochitests should work.
Assignee: nobody → jlong
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
This new patch includes this PR https://github.com/devtools-html/debugger.html/commit/e89e5ce569a1ed7e967789ab2bd65984951f6894 which loads React locally from m-c when in the bundle and doesn't bundle it in. I happened to be trying to reproduce the leak with this and couldn't, and then tried a version without that commit and I could repro the leak. I have no idea why using a local React is causing a leak; the only thing I can think of it that it's a false positive and maybe it takes a little longer for the window to shut down or something. Not sure, but this should magically go away...
Attachment #8789536 -
Attachment is obsolete: true
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8789591 -
Attachment is obsolete: true
Comment 4•8 years ago
|
||
Try push with this + the pref on by default (expecting a bunch of failures here as in Bug 1300861): https://treeherder.mozilla.org/#/jobs?repo=try&revision=143f48376745 Try push with just this patch (to make sure leak is gone and to see if this is landable): https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca3fd2491099
Assignee | ||
Comment 6•8 years ago
|
||
This is the latest patch which is officially what I was after for the next debugger update. It removes React and CodeMirror from our bundle and adds a bunch of mochitests, which are the most important things.
Attachment #8789825 -
Attachment is obsolete: true
Assignee | ||
Comment 7•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09df0a06e35d
Assignee | ||
Comment 8•8 years ago
|
||
New try push! Hopefully this fixes a faulty new test. If not we'll disable it for now (it was just merged today) https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd58ab863a32
Assignee | ||
Comment 9•8 years ago
|
||
Some errors, another push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e9fa3e7b368
Assignee | ||
Comment 10•8 years ago
|
||
Two new try pushes: This one disables the pause on exceptions tests, as it seems faulty: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2a60de1f7de This one implements a proper panel shutdown method which kills the sourcemap worker: https://hg.mozilla.org/try/rev/248817780e4b I'm seeing some weird memory leaks and we know the worker is leaky, so want to make sure it's killed. Previously it was using an "unload" event to try to kill it.
Assignee | ||
Comment 11•8 years ago
|
||
A few more attempts. I think there was a problem with using the local CodeMirror; it needs to be properly shutdown. Here is a new push with that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=640607aeed63 This second push is the same except with the iframes and pause on exceptions tests disabled. I'm skeptical that those tests were failing before because of the codemirror issue, so it's likely those tests are faulty. If this tests passes, I'm inclined to disable those tests so that we can land this update. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a34189be6b29
Assignee | ||
Comment 12•8 years ago
|
||
Adding this here so we can commit all this at once. We need to introduce the `destroy` method on the new debugger at the same time.
Comment 14•8 years ago
|
||
Pushed by jlong@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/b8cba97b2648 Update the debugger bundle and implement a proper shutdown method r=me UPDATE_BUNDLE
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b8cba97b2648
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Comment 16•8 years ago
|
||
Will updates to the debugger frontend be imported from https://github.com/devtools-html/debugger.html once in a while or should I file bugs related to the new frontend here in Bugzilla? Sebastian
Flags: needinfo?(jlong)
Assignee | ||
Comment 17•8 years ago
|
||
@sebo yes, we will import it either weekly or several times a week, so please report issues over there.
Flags: needinfo?(jlong)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•