Closed Bug 1408562 Opened 7 years ago Closed 7 years ago

Update Debugger Frontend (10-13)

Categories

(DevTools :: Debugger, enhancement, P3)

Product:
DevTools
Component:
Debugger
Version:
57 Branch
Type:
enhancement

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: jlast, Assigned: jlast)

References

Details

Attachments

(1 file, 11 obsolete files)

patch-10-13-31.patch
2.79 MB, patch
jdescottes
: review+
Details | Diff | Splinter Review 
      No description provided.
Attached patch patch-10-13-1.patch (obsolete) — — 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

    
    

    
  
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a52945a497c8
Update Debugger frontend (10-13). r=jdescottes
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)
        — 
          — 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)
        — 
          — 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)
        — 
          — 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)
        — 
          — 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)
        — 
          — 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)
        — 
          — 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)
        — 
          — 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)
        — 
          — 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)
        — 
          — 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)
        — 
          — Splinter Review
      

    


  

        Attachment #8919893 -
        Flags: review?(jdescottes)

    
  

        Attachment #8919893 -
        Attachment is obsolete: true

        Attachment #8919893 -
        Flags: review?(jdescottes)

    
    

    
  


  

        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+

    
    

    
  
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47bb399c0621
Update Debugger frontend (10-13). r=jdescottes

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/47bb399c0621
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox58:
          --- → fixed
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

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: