1719615 - Do not turn on debug code for asm.js/wasm just because the console is open

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Lars T Hansen [:lth]]

+++ This bug was initially created as a clone of Bug #1654590 +++

See bug 1654590 comment 4 and bug 1654590 comment 5: Opening the console must not turn on debugging for asm.js/wasm as this seriously pessimizes the code and makes even printf debugging hard to use. In the absence of good devtools support for wasm, printf debugging needs to work well.

Comment 1

[Julian Descottes [:jdescottes]]

There is a test case in Bug 1656549 to reproduce the issue here.

lth: We will review the current support for WASM in the debugger to decide if we should just turn off wasm support by default. From there we'll either add a preference or a UI to toggle WASM support. From your point of view, how is the devtools support right now? Doesn't work at all? Only works on simple examples? We seem to have a few mochitests around, so I assume "some" things still work, but having your feedback will be helpful. Thanks!

Comment 2

[Lars T Hansen [:lth]]

Hi Julian, thanks for reaching out.

Debugging support for wasm is clearly only partial, as we have no source-level debugging. (Customers are complaining about this but unless we put a lot of effort into supporting DWARF there's nothing we can really do to support source-level debugging; customers are currently switching to Chrome to debug wasm at the source level.) But we do have correct call stacks and we can single-step bytecodes in the debugger, and for those who know what they're looking at this is at least something. The call stacks in particular are valuable for narrowing down problems.

My general impression is that what we have works the way it should, and that it's better than nothing, sometimes much better than nothing.

From my perspective it would be fairly meaningful to enable wasm/asm.js debugging when the user switches to the Debugger tab in the console. At that point the page must often be reloaded anyway, and wasm debug code will be generated properly. Switching on wasm debugging when simply displaying the console is however problematic, due to the major slowdown / size increase of the code. The patch on bug 1714086, now pending, will mitigate the bloat but not the slowdown.

We probably ought to discuss whether asm.js and wasm have different default behaviors here, as enabling debugging for asm.js routes asm.js code through the JS compiler -- normally it's routed through the wasm compiler -- while enabling debugging for wasm just selects a different wasm compiler for the code. But I suspect that enabling debugging for asm.js when switching to the console, as opposed to the debugger, is also not the best way to handle it.

(I'm about to go out on summer holiday, back in August; perhaps we can sync about the matter then. My gut feeling right now is that a preference is not desirable; UI is better, if we need the user to control this at all. I'm open to arguments. We might include Yury, cc'd.)

Comment 5

[Yury Delendik (:yury)]

Attachment: Bug 1719615 - Add observeWasm in addition to observeAsmJS.

Comment 6

[Yury Delendik (:yury)]

In the getThreadOptions() at thread-utils.js, we are blindly turn on observeAsmJS. We can try to disconnect, and turn observeWasm only when debugger tab is active.

Comment 7

[Yury Delendik (:yury)]

Attachment: WIP: Bug 1719615 - observeWasm only when the debugger tab was activated.

Depends on D123626

Comment 8

[Yury Delendik (:yury)]

The WIP patches disable blanket enabling of debugging mode when JS Debugger is attached to the thread. It has to be explicitly enabled with "observeWasm" configuration. It is only done when the debugger tab/tool is active. Is it an acceptable approach?

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Ah, this fell off my radar, sorry about that.
Is this bug still active, should I find someone to help with it?

Honza

Comment 10

[Lars T Hansen [:lth]]

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #9)

Ah, this fell off my radar, sorry about that.
Is this bug still active, should I find someone to help with it?

Yury will provide a definitive answer when he wakes up, but I believe we're still running into this and the consequences are a little more dire than they used to be, as some content simply won't work if we generate debug code because the code is too large (bug 1756030). In that case, turning on debug code when the console is open prevents any kind of debugging at all, even printf / console.log debugging.

It would be great to have a name to contact for the devtools side of things of this.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the update. Note that there are some requested changes from :ochameau (Alexandre Poirot, DevTools team contact) in the attached patch. https://phabricator.services.mozilla.com/D123665#4033193

Comment 12

[Yury Delendik (:yury)]

Note that there are some requested changes from :ochameau (Alexandre Poirot, DevTools team contact) in the attached patch. https://phabricator.services.mozilla.com/D123665

Sorry, I'm not familiar with current code base. I closed my wip, and it looks like more help is needed from DevTools team to address the issue.

Comment 13

[Lars T Hansen [:lth]]

I'd like to have the priority/severity of this adjusted, if possible. As noted in comment 10, the problem prevents all debugging of large wasm-using applications, even the non-wasm and non-JS parts. The dependent bug 1756030 is now a P2/S2.

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I see, adding this to our list for team triage meeting today.

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1719615 - [devtools] Enable 'observeWasm' only once the debugger is opened.

Comment 16

[Alexandre Poirot [:ochameau]]

So I crafted https://phabricator.services.mozilla.com/D140069 in order to toggle observeAsmJS only once the debugger opens.
But given that my knowledge around WASM is close to zero, i have very few opinion about this patch.
I asked for review to Yury, but do not hesitate to involve whoever is meaninful here.

If you have online example of complex app, or typical usecase you would like to see fixed that would be super helpful.
I'd like to add some test coverage, but I don't quite picture how things should work in the console and debugger.
It would be nice to have some test to explicitely assert the shortcomings of this patch.
For ex:

• Is there anything in the console that will stop working or work differently now that observeAsmJS is off?
• Similar question against the debugger. Now that observeAsmJS is turned on late, would that introduce any artefact in the debugger?

I imagine observeAsmJS set to true will only have an impact when we reload the page after having turned the flag?
Turning it on without reloading won't enable anything?

Comment 17

[Lars T Hansen [:lth]]

Thanks! I'll let Yury do a first round of review. In principle, there's a difference here between Wasm and asm.js. When we turn on debugging for asm.js, the engine falls back to the JS compiler, and the existing behavior is benign (and arguably even desired since asm.js is really also just JS and one then gets debugging of asm.js code via the JS debugging pipeline). When we turn on debugging for Wasm, however, the engine generates different machine code to allow debugging, and this is the code that blows the memory budget. So it's possible we want to have two different switches, one for asm.js and one for Wasm, and leave the asm.js behavior as it was. We can discuss this once the patch has been reviewed.

Regarding test cases, try this:

With the console closed:

1. Go to photoshop
2. You should see a screen asking you to select a local image, do so
3. You should see a picture editing screen (borders etc) before the app stops working
4. If you open the console, you should see errors about some DOM functionality missing

Now repeat with the console open before you load the site and you should start seeing errors about out-of-memory. (That's without your patch.) If your patch works, you should instead see the errors about missing DOM functionality.

Also see https://bugzilla.mozilla.org/show_bug.cgi?id=1756030#c3 for a different test case.

Comment 18

[Alexandre Poirot [:ochameau]]

(In reply to Lars T Hansen [:lth] from comment #17)

Now repeat with the console open before you load the site and you should start seeing errors about out-of-memory. (That's without your patch.) If your patch works, you should instead see the errors about missing DOM functionality.

I confirm. Without the patch, while having only the console opened before reload, I get asm errors:
TypeError: can't access property "_emscripten_thread_init", Module.asm is undefined
Same STR, with the patch I now see the DOM errors (and no longer any asm error):
TypeError: navigator.storage.getDirectory is not a function apollo_web.d8c29252826c93c42f38.js:1:139955

Thanks for the test cases, that's helpful to see the real thing we would like to see working!

I notice that on photoshop, the debugger looks completely lost. No source are displayed at all!

Comment 19

[Yury Delendik (:yury)]

Attachment: small test case with worker

Comment 20

[Yury Delendik (:yury)]

I attached smaller test case:

STR:

1. Open index.html, e.g. via http://localhost:8080/, without devtools opened
2. After 10 seconds, open devtools console inspect messages, open debugerg tab, inspect wasm sources
3. Close devtools, reload page, wait 10 sec
4. Open devtools directly at the console tab, reload the page
5. After 10 seconds, open debugger tab, inspect wasm sources
6. Close devtools, reload page, wait 10 sec
7. Open devtools directly at the debugger tab, reload the page
8. After 10 seconds, inspect wasm sources

Expected that only at step 8 wasm sources are available.

Currently, even with D140069, sources are available at step 5 -- which means Wasm engine is in debug mode.

Comment 21

[Alexandre Poirot [:ochameau]]

(In reply to Yury Delendik (:yury) from comment #20)

Currently, even with D140069, sources are available at step 5 -- which means Wasm engine is in debug mode.

Oops, sorry I missed that D123626 was toggling observeWasm in many places.
The two toggles set in make-debugger.js and thread-utils.js should be reverted.
I reverted them, but feel free to do so in your patch. Feel free also to inline my changeset into yours.

Comment 22

[Yury Delendik (:yury)]

(In reply to Alexandre Poirot [:ochameau] from comment #21)

(In reply to Yury Delendik (:yury) from comment #20)
The two toggles set in make-debugger.js and thread-utils.js should be reverted.

I reverted them locally. The problem still exists: step 5 from STR above has wasm source available.

I don't feel comfortable reviewing code in devtools/ directory. Can you add a test similar to the STR? I will be more comfortable to review that.

Comment 23

[Yury Delendik (:yury)]

Attachment: WIP: Bug 1719615 - Test case for wasm source avialability.

Comment 24

[Yury Delendik (:yury)]

I opened pilot D140773 with simplified, without wasm workers, test based on comment 20. It is failing for current Firefox and D140069 (though passes for abandoned D123665).

Alexandre, can you add the similar test to your D140069 patch?

Comment 25

[Alexandre Poirot [:ochameau]]

I got confused by the way these "unobserve" flags were working. I should not have removed the attribute set to true in make-debugger.js.
I fixed that and will try to provide some test for both asm.js and wasm. I already started doing something for asm.js in D140195.
The test now pass for me and I also verified that comment 20 STR was correct.

Comment 26

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1719615 - [devtools] Merge existing wasm tests into a unique "features" test.

The sourcemaps one was disabled.
In the new test, I'm now trying to assert the intermediate behavior of the debugger
when we haven't reloaded the page yet.
The test now also assert much more things about the content being displayed.
And also check for breakable lines.

Comment 28

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 3 duplicates.
:yury, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 29

[Yury Delendik (:yury)]

Right, I'm still assigned to this bug, though Alexandre, did the part of the work. Increasing priority and severity.

ochameau, free to assign the bug and land all the patches at your convenience.

Comment 30

[Alexandre Poirot [:ochameau]]

The test added in D141334 triggers a crash from Debugger's findSources which is called from there by DevTools codebase:
https://searchfox.org/mozilla-central/rev/840881e1232f664a58b39caaae6284c7bcf121df/devtools/server/actors/utils/sources-manager.js#159-162

I think it crashes around these lines, but I need a debug build to confirm:
https://searchfox.org/mozilla-central/rev/840881e1232f664a58b39caaae6284c7bcf121df/js/src/debugger/Debugger.cpp#5617-5628

The artifact builds aren't providing enough debug infos:
0 0x00007fd1d7a0b88d in js::wasm::Instance::object() const () at /mnt/desktop/gecko-dev/obj-firefox-artifact-debug/dist/bin/libxul.so
#1 0x00007fd1d7038940 in js::Debugger::SourceQuery::findSources() () at /mnt/desktop/gecko-dev/obj-firefox-artifact-debug/dist/bin/libxul.so
#2 0x00007fd1d7037da3 in js::Debugger::CallData::findSources() () at /mnt/desktop/gecko-dev/obj-firefox-artifact-debug/dist/bin/libxul.so

Comment 31

[Alexandre Poirot [:ochameau]]

So the actual stack trace is:

#0  js::gc::detail::CellHasStoreBuffer(js::gc::Cell const*) (cell=0x33b50fae823800) at /mnt/desktop/gecko-dev/obj-firefox-opt/dist/include/js/HeapAPI.h:590
#1  js::gc::IsInsideNursery(js::gc::Cell const*) (cell=0x33b50fae823800) at /mnt/desktop/gecko-dev/obj-firefox-opt/dist/include/js/HeapAPI.h:599
#2  js::gc::Cell::isTenured() const (this=0x33b50fae823800) at /mnt/desktop/gecko-dev/js/src/gc/Cell.h:151
#3  js::gc::ReadBarrierImpl(js::gc::Cell*) (thing=0x33b50fae823800) at /mnt/desktop/gecko-dev/js/src/gc/Cell.h:501
#4  js::gc::ReadBarrier<js::WasmInstanceObject>(js::WasmInstanceObject*) (thing=0x33b50fae823800) at /mnt/desktop/gecko-dev/js/src/gc/Cell.h:470
#5  js::InternalBarrierMethods<js::WasmInstanceObject*, void>::readBarrier(js::WasmInstanceObject*) (v=0x33b50fae823800) at /mnt/desktop/gecko-dev/js/src/gc/Barrier.h:353
#6  js::ReadBarriered<js::WasmInstanceObject*>::read() const (this=0x7f2cd41785df) at /mnt/desktop/gecko-dev/js/src/gc/Barrier.h:842
#7  js::WeakHeapPtr<js::WasmInstanceObject*>::get() const (this=0x7f2cd41785df) at /mnt/desktop/gecko-dev/js/src/gc/Barrier.h:895
#8  js::WeakHeapPtr<js::WasmInstanceObject*>::operator js::WasmInstanceObject* const&() const (this=0x7f2cd41785df) at /mnt/desktop/gecko-dev/js/src/gc/Barrier.h:904
#9  js::wasm::Instance::object() const (this=0x7f2cd417855f) at /mnt/desktop/gecko-dev/js/src/wasm/WasmInstance.cpp:1862
#10 0x00007f2cdf7f6dbf in js::Debugger::SourceQuery::findSources() (this=<optimized out>, this@entry=0x7ffd4d7a4b00)
    at /mnt/desktop/gecko-dev/js/src/debugger/Debugger.cpp:5676

A segfault happens in the following code from Debugger's findSources:
https://searchfox.org/mozilla-central/rev/1f617334179cf28b4b310d1d116ddbc8ef3348ea/js/src/debugger/Debugger.cpp#5617-5628

// TODO: Until such time that wasm modules are real ES6 modules,
// unconditionally consider all wasm toplevel instance scripts.
for (WeakGlobalObjectSet::Range r = debugger->allDebuggees(); !r.empty();
     r.popFront()) {
  for (wasm::Instance* instance : r.front()->realm()->wasm.instances()) {
    consider(instance->object());
    if (oom) {
      ReportOutOfMemory(cx);
      return false;
    }
  }
}

There is something wrong with wasm::Instance* instance, we get a crash when accessing instance->object().
Unfortunately, my C++ knowledges are too limited to be able to know what's wrong here.
Could it be that we manipulate an object that has been GC-ed?

The crash reproduces when applying all bug's patches and running:

$ ./mach mochitest --headless devtools/client/debugger/test/mochitest/browser_dbg-features-wasm.js

Comment 32

[Yury Delendik (:yury)]

I'm not sure yet what is going on yet. My crash signature unknown and points to unknown/JIT code. It happens after "Register and trigger a breakpoint from the original source in C". To test functionality of this bug, we don't need step or debug wasm code. Can we reduce test to minimally test availability of the wasm text in this bug without initiating the entire debugging session?

I would like to move the enabling of browser_dbg-wasm-sourcemaps.js into different bug. There might be a reason why it was disabled in first place, and I'm thinking we are hitting just it. Disabling of debugging with opened console is a higher priority and we need to test just that.

Comment 34

[Alexandre Poirot [:ochameau]]

The crash relates to using emscripten in the test page.
I switched to use WASI SDK + emscripten's wasm-sourcemap.py script so that I'm still close to what someone could be doing online.
This no longer crash locally. Also I tweaked the test to open first on the console and reload one, so that it fails without this fix.

Regarding the crash it wasn't crashing when I started working on this test.
The crash seems to have been introduced by bug 1756951.

If try is green, I'll land the queue:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cab9e7cb4685a7d6c01c9d6a0e7c0c3c31e6290

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/238cb293027b
Add observeWasm in addition to observeAsmJS. r=lth
https://hg.mozilla.org/integration/autoland/rev/093bfc2ab581
[devtools] Enable 'observeWasm' only once the debugger is opened. r=yury,bomsy
https://hg.mozilla.org/integration/autoland/rev/9a7ecb82d163
[devtools] Merge existing wasm tests into a unique "features" test. r=bomsy,yury

Comment 36

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/238cb293027b
https://hg.mozilla.org/mozilla-central/rev/093bfc2ab581
https://hg.mozilla.org/mozilla-central/rev/9a7ecb82d163