Closed Bug 1408562 Opened 7 years ago Closed 7 years ago

Update Debugger Frontend (10-13)

Categories

(DevTools :: Debugger, enhancement, P3)

57 Branch
enhancement

Tracking

(firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: jlast, Assigned: jlast)

References

Details

Attachments

(1 file, 11 obsolete files)

No description provided.
Attached patch patch-10-13-1.patch (obsolete) (deleted) — Splinter Review
Attachment #8918465 - Flags: review?(jdescottes)
Assignee: nobody → jlaster
Severity: normal → enhancement
Status: NEW → ASSIGNED
Priority: -- → P3
I think the try failures are linked to the fact that we used --artifact for the try run. We introduced new telemetry probes in https://bugzilla.mozilla.org/show_bug.cgi?id=1405584 and I guess the artifact build retrieved did not have the probes. Here are 2 new runs: - with artifact https://treeherder.mozilla.org/#/jobs?repo=try&revision=5390422469f144d21bce95cb21c6659b47abf5f4 - without artifact https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5036f2157b255e239e5facba762b251b2801f6d
Comment on attachment 8918465 [details] [diff] [review] patch-10-13-1.patch Review of attachment 8918465 [details] [diff] [review]: ----------------------------------------------------------------- R+ if the try runs above are green. Fixes the breapoint issue raised during previous bundle review, thanks!
Attachment #8918465 - Flags: review?(jdescottes) → review+
New try with --artifact is green.
Keywords: checkin-needed
Keywords: checkin-needed
Backed out for leak in dt1 job on Linux x64 asan: https://hg.mozilla.org/integration/mozilla-inbound/rev/e862d9aa21e366753220bbcc1f9c9aaacace95d5 Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a52945a497c89cca6b8edb746837218dbd64e61d&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=137271899&repo=mozilla-inbound [task 2017-10-16T20:22:03.338Z] 20:22:03 ERROR - GECKO(1095) | ==1232==ERROR: LeakSanitizer: detected memory leaks [task 2017-10-16T20:22:03.342Z] 20:22:03 INFO - GECKO(1095) | Direct leak of 56 byte(s) in 1 object(s) allocated from: [task 2017-10-16T20:22:03.345Z] 20:22:03 INFO - GECKO(1095) | #0 0x4bc44c in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64:3 [task 2017-10-16T20:22:03.382Z] 20:22:03 INFO - GECKO(1095) | #1 0x7f119be53c98 in js_malloc /builds/worker/workspace/build/src/obj-firefox/dist/include/js/Utility.h:358:12 [task 2017-10-16T20:22:03.402Z] 20:22:03 INFO - GECKO(1095) | #2 0x7f119be53c98 in js_pod_malloc<unsigned char> /builds/worker/workspace/build/src/obj-firefox/dist/include/js/Utility.h:549 [task 2017-10-16T20:22:03.410Z] 20:22:03 INFO - GECKO(1095) | #3 0x7f119be53c98 in maybe_pod_malloc<unsigned char> /builds/worker/workspace/build/src/js/src/vm/MallocProvider.h:54 [task 2017-10-16T20:22:03.414Z] 20:22:03 INFO - GECKO(1095) | #4 0x7f119be53c98 in unsigned char* js::MallocProvider<JS::Zone>::pod_malloc<unsigned char>(unsigned long) /builds/worker/workspace/build/src/js/src/vm/MallocProvider.h:87 [task 2017-10-16T20:22:03.421Z] 20:22:03 INFO - GECKO(1095) | #5 0x7f119d2db48c in new_<js::WasmBreakpointSite, js::wasm::DebugState *, unsigned int &> /builds/worker/workspace/build/src/js/src/vm/MallocProvider.h:187:5 [task 2017-10-16T20:22:03.442Z] 20:22:03 INFO - GECKO(1095) | #6 0x7f119d2db48c in js::wasm::DebugState::getOrCreateBreakpointSite(JSContext*, unsigned int) /builds/worker/workspace/build/src/js/src/wasm/WasmDebug.cpp:420 [task 2017-10-16T20:22:03.460Z] 20:22:03 INFO - GECKO(1095) | #7 0x7f119ccfd7e3 in js::Debugger::onTrap(JSContext*, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Debugger.cpp:2049:53 [task 2017-10-16T20:22:03.464Z] 20:22:03 INFO - GECKO(1095) | #8 0x7f119d2bd5d5 in WasmHandleDebugTrap() /builds/worker/workspace/build/src/js/src/wasm/WasmBuiltins.cpp:147:31 [task 2017-10-16T20:22:03.474Z] 20:22:03 INFO - GECKO(1095) | ----------------------------------------------------- [task 2017-10-16T20:22:03.476Z] 20:22:03 INFO - GECKO(1095) | Suppressions used: [task 2017-10-16T20:22:03.479Z] 20:22:03 INFO - GECKO(1095) | count bytes template [task 2017-10-16T20:22:03.488Z] 20:22:03 INFO - GECKO(1095) | 8 256 libc.so [task 2017-10-16T20:22:03.496Z] 20:22:03 INFO - GECKO(1095) | 789 24912 nsComponentManagerImpl [task 2017-10-16T20:22:03.500Z] 20:22:03 INFO - GECKO(1095) | 6 1056 mozJSComponentLoader::LoadModule [task 2017-10-16T20:22:03.510Z] 20:22:03 INFO - GECKO(1095) | 611 17713 libfontconfig.so [task 2017-10-16T20:22:03.521Z] 20:22:03 INFO - GECKO(1095) | 12 416 style::gecko::global_style_data [task 2017-10-16T20:22:03.524Z] 20:22:03 INFO - GECKO(1095) | 1 72 nss_ClearErrorStack [task 2017-10-16T20:22:03.529Z] 20:22:03 INFO - GECKO(1095) | 16 2316 libglib-2.0.so [task 2017-10-16T20:22:03.544Z] 20:22:03 INFO - GECKO(1095) | ----------------------------------------------------- [task 2017-10-16T20:22:03.548Z] 20:22:03 INFO - GECKO(1095) | SUMMARY: AddressSanitizer: 56 byte(s) leaked in 1 allocation(s). [task 2017-10-16T20:22:07.218Z] 20:22:07 INFO - GECKO(1095) | ----------------------------------------------------- [task 2017-10-16T20:22:07.220Z] 20:22:07 INFO - GECKO(1095) | Suppressions used: [task 2017-10-16T20:22:07.223Z] 20:22:07 INFO - GECKO(1095) | count bytes template [task 2017-10-16T20:22:07.227Z] 20:22:07 INFO - GECKO(1095) | 710 22616 nsComponentManagerImpl [task 2017-10-16T20:22:07.232Z] 20:22:07 INFO - GECKO(1095) | 50 8800 mozJSComponentLoader::LoadModule [task 2017-10-16T20:22:07.234Z] 20:22:07 INFO - GECKO(1095) | 1 384 pixman_implementation_lookup_composite [task 2017-10-16T20:22:07.236Z] 20:22:07 INFO - GECKO(1095) | 612 17514 libfontconfig.so [task 2017-10-16T20:22:07.238Z] 20:22:07 INFO - GECKO(1095) | 2 64 libcairo.so [task 2017-10-16T20:22:07.240Z] 20:22:07 INFO - GECKO(1095) | 1 32 libdl.so [task 2017-10-16T20:22:07.242Z] 20:22:07 INFO - GECKO(1095) | 17 4348 libglib-2.0.so [task 2017-10-16T20:22:07.244Z] 20:22:07 INFO - GECKO(1095) | 1 40 libpulsecommon-8.0.so [task 2017-10-16T20:22:07.246Z] 20:22:07 INFO - GECKO(1095) | ----------------------------------------------------- [task 2017-10-16T20:22:07.624Z] 20:22:07 INFO - TEST-INFO | Main app process: exit 0 [task 2017-10-16T20:22:07.627Z] 20:22:07 INFO - TEST-INFO | LeakSanitizer | To show the addresses of leaked objects add report_objects=1 to LSAN_OPTIONS [task 2017-10-16T20:22:07.631Z] 20:22:07 INFO - TEST-INFO | LeakSanitizer | This can be done in testing/mozbase/mozrunner/mozrunner/utils.py [task 2017-10-16T20:22:07.635Z] 20:22:07 ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, js::wasm::DebugState::getOrCreateBreakpointSite, js::Debugger::onTrap
Flags: needinfo?(jdescottes)
Sorry about that. Looks like browser_dbg-wasm-sourcemaps.js (which was just unskipped in this Bug) surfaced a leak. Stack trace: [task 2017-10-16T20:22:03.338Z] 20:22:03 ERROR - GECKO(1095) | ==1232==ERROR: LeakSanitizer: detected memory leaks [task 2017-10-16T20:22:03.342Z] 20:22:03 INFO - GECKO(1095) | Direct leak of 56 byte(s) in 1 object(s) allocated from: [task 2017-10-16T20:22:03.345Z] 20:22:03 INFO - GECKO(1095) | #0 0x4bc44c in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64:3 [task 2017-10-16T20:22:03.382Z] 20:22:03 INFO - GECKO(1095) | #1 0x7f119be53c98 in js_malloc /builds/worker/workspace/build/src/obj-firefox/dist/include/js/Utility.h:358:12 [task 2017-10-16T20:22:03.402Z] 20:22:03 INFO - GECKO(1095) | #2 0x7f119be53c98 in js_pod_malloc<unsigned char> /builds/worker/workspace/build/src/obj-firefox/dist/include/js/Utility.h:549 [task 2017-10-16T20:22:03.410Z] 20:22:03 INFO - GECKO(1095) | #3 0x7f119be53c98 in maybe_pod_malloc<unsigned char> /builds/worker/workspace/build/src/js/src/vm/MallocProvider.h:54 [task 2017-10-16T20:22:03.414Z] 20:22:03 INFO - GECKO(1095) | #4 0x7f119be53c98 in unsigned char* js::MallocProvider<JS::Zone>::pod_malloc<unsigned char>(unsigned long) /builds/worker/workspace/build/src/js/src/vm/MallocProvider.h:87 [task 2017-10-16T20:22:03.421Z] 20:22:03 INFO - GECKO(1095) | #5 0x7f119d2db48c in new_<js::WasmBreakpointSite, js::wasm::DebugState *, unsigned int &> /builds/worker/workspace/build/src/js/src/vm/MallocProvider.h:187:5 [task 2017-10-16T20:22:03.442Z] 20:22:03 INFO - GECKO(1095) | #6 0x7f119d2db48c in js::wasm::DebugState::getOrCreateBreakpointSite(JSContext*, unsigned int) /builds/worker/workspace/build/src/js/src/wasm/WasmDebug.cpp:420 [task 2017-10-16T20:22:03.460Z] 20:22:03 INFO - GECKO(1095) | #7 0x7f119ccfd7e3 in js::Debugger::onTrap(JSContext*, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Debugger.cpp:2049:53 [task 2017-10-16T20:22:03.464Z] 20:22:03 INFO - GECKO(1095) | #8 0x7f119d2bd5d5 in WasmHandleDebugTrap() /builds/worker/workspace/build/src/js/src/wasm/WasmBuiltins.cpp:147:31
Flags: needinfo?(jdescottes)
Attachment #8918465 - Attachment is obsolete: true
Attached patch patch-10-13-3.patch (obsolete) (deleted) — Splinter Review
Attachment #8919085 - Flags: review?(jlaster)
disregard this patch...
Attachment #8919085 - Attachment is obsolete: true
Attachment #8919085 - Flags: review?(jlaster)
Attached patch patch-10-13-4.patch (obsolete) (deleted) — Splinter Review
Attachment #8919098 - Flags: review?(jlaster)
Attachment #8919098 - Attachment is obsolete: true
Attachment #8919098 - Flags: review?(jlaster)
Attached patch patch-10-13-7.patch (obsolete) (deleted) — Splinter Review
Attachment #8919359 - Flags: review?(jlaster)
Attachment #8919359 - Attachment is obsolete: true
Attachment #8919359 - Flags: review?(jlaster)
Attached patch patch-10-13-8.patch (obsolete) (deleted) — Splinter Review
Attachment #8919361 - Flags: review?(jlaster)
Attachment #8919361 - Attachment is obsolete: true
Attachment #8919361 - Flags: review?(jlaster)
Attached patch patch-10-13-10.patch (obsolete) (deleted) — Splinter Review
Attachment #8919371 - Flags: review?(jlaster)
Comment on attachment 8919371 [details] [diff] [review] patch-10-13-10.patch Review of attachment 8919371 [details] [diff] [review]: ----------------------------------------------------------------- Something is wrong, have you based your patch off Beta? The diff is huge and contains a lot of unrelated changes. Also the r? points to you.
Attachment #8919371 - Flags: review?(jlaster) → review-
Attachment #8919371 - Attachment is obsolete: true
Attached patch patch-10-13-11.patch (obsolete) (deleted) — Splinter Review
Attachment #8919376 - Flags: review?(jlaster)
Attachment #8919376 - Attachment is obsolete: true
Attachment #8919376 - Flags: review?(jlaster)
Attached patch patch-10-13-21.patch (obsolete) (deleted) — Splinter Review
Attachment #8919741 - Flags: review?(jlaster)
Comment on attachment 8919741 [details] [diff] [review] patch-10-13-21.patch Review of attachment 8919741 [details] [diff] [review]: ----------------------------------------------------------------- As discussed, icons are missing in the status bar (blackboxing + prettify) CSS points to: - chrome://devtools/skin/images/debugger/prettyPrint.svg - chrome://devtools/skin/images/debugger/blackBox.svg Both missing from the patch Other than this, didn't see anything broken here. R+ with green try + fixed reviewer in commit message. Side note: the last patch is almost 3MB big (the bundle seems to be shuffled around a lot). The first patch was less than 200kB. Just mentioning it in case this was unexpected.
Attachment #8919741 - Flags: review?(jlaster) → review+
Attachment #8919741 - Attachment is obsolete: true
Attached patch patch-10-13-24.patch (obsolete) (deleted) — Splinter Review
Attachment #8919864 - Flags: review?(jdescottes)
Attachment #8919864 - Attachment is obsolete: true
Attachment #8919864 - Flags: review?(jdescottes)
Attached patch patch-10-13-26.patch (obsolete) (deleted) — Splinter Review
Attachment #8919878 - Flags: review?(jdescottes)
Comment on attachment 8919878 [details] [diff] [review] patch-10-13-26.patch Review of attachment 8919878 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8919878 - Flags: review?(jdescottes) → review+
Attachment #8919878 - Attachment is obsolete: true
Attached patch patch-10-13-30.patch (obsolete) (deleted) — Splinter Review
Attachment #8919893 - Flags: review?(jdescottes)
Attachment #8919893 - Attachment is obsolete: true
Attachment #8919893 - Flags: review?(jdescottes)
Attached patch patch-10-13-31.patch (deleted) — Splinter Review
Attachment #8919975 - Flags: review?(jdescottes)
Comment on attachment 8919975 [details] [diff] [review] patch-10-13-31.patch Review of attachment 8919975 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8919975 - Flags: review?(jdescottes) → review+
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
editor.addConditionalBreakpoint string content has been changed without id update, https://hg.mozilla.org/mozilla-central/diff/47bb399c0621/devtools/client/locales/en-US/debugger.properties
Flags: needinfo?(francesco.lodolo)
(In reply to Stefan Plewako [:stef] from comment #39) > editor.addConditionalBreakpoint string content has been changed without id > update, > https://hg.mozilla.org/mozilla-central/diff/47bb399c0621/devtools/client/ > locales/en-US/debugger.properties Please ignore it, it's already been reverted in GitHub and will return to the previous value in the next export. https://github.com/devtools-html/debugger.html/commit/e5e246fa6cd7521b7a7773be4da71acff2bdfd47 Since we have been stuck for over a week in exposing new strings, I decided to not block further because of this change.
Flags: needinfo?(francesco.lodolo)
It's being reverted in Bug 1411727
https://hg.mozilla.org/mozilla-central/diff/47bb399c0621/devtools/client/locales/en-US/debugger.properties > # LOCALIZATION NOTE (expressions.placeholder): Placeholder text for expression > # input element > expressions.placeholder=Add watch expression > +expressions.placeholder.accesskey=e Is it possible now or sth is wrong here?
(In reply to Stefan Plewako [:stef] from comment #42) > https://hg.mozilla.org/mozilla-central/diff/47bb399c0621/devtools/client/ > locales/en-US/debugger.properties > > # LOCALIZATION NOTE (expressions.placeholder): Placeholder text for expression > > # input element > > expressions.placeholder=Add watch expression > > +expressions.placeholder.accesskey=e > > Is it possible now or sth is wrong here? Likely. Either the comment is completely off (no accesskey on a placeholder), or the string is used in multiple places. Seems the latter https://github.com/devtools-html/debugger.html/blob/6660e0ddd273fb09e8b8a725a299a6f601279482/src/components/SecondaryPanes/Expressions.js#L182 https://github.com/devtools-html/debugger.html/blob/4114a97f1584097c64d85b3d02e51946f4533caf/src/components/Editor/EditorMenu.js#L91
(In reply to Francesco Lodolo [:flod] from comment #43) > (In reply to Stefan Plewako [:stef] from comment #42) > > https://hg.mozilla.org/mozilla-central/diff/47bb399c0621/devtools/client/ > > locales/en-US/debugger.properties > > > # LOCALIZATION NOTE (expressions.placeholder): Placeholder text for expression > > > # input element > > > expressions.placeholder=Add watch expression > > > +expressions.placeholder.accesskey=e > > > > Is it possible now or sth is wrong here? > > Likely. Either the comment is completely off (no accesskey on a > placeholder), or the string is used in multiple places. https://github.com/devtools-html/debugger.html/issues/4513 Thanks for catching it.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: