Closed Bug 1333840 Opened 7 years ago Closed 7 years ago

wasm frames cause exceptions at debugger server actors code

Categories

(DevTools :: Debugger, defect)

Product:
DevTools
Component:
Debugger
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: yury, Assigned: yury)

References

Details

Attachments

(1 file)

Bug 1333840 - Protect from frame.this access for wasm debugger frame.
59 bytes, text/x-review-board-request
ochameau
: review+
Details 
Comment on attachment 8830413 [details]
Bug 1333840 - Protect from frame.this access for wasm debugger frame.

https://reviewboard.mozilla.org/r/107188/#review108532

::: devtools/server/actors/frame.js:73
(Diff revision 1)
>        );
>        form.environment = envActor.form();
>      }
> -    form.this = createValueGrip(this.frame.this, threadActor._pausePool,
> +    var frameThis = this.frame.type == "wasmcall" ? undefined : this.frame.this;
> +    form.this = createValueGrip(frameThis, threadActor._pausePool,
>        threadActor.objectGrip);

I'm wondering if we can omit setting form.this at all? The client may start throwing if we don't...

::: devtools/server/actors/script.js:2199
(Diff revision 1)
> +    if (this.type != "wasm") {
> -    if (this.url) {
> +      if (this.url) {
> -      output += this.url;
> +        output += this.url;
> -    }
> +      }
> +    } else {
> +      output += this.displayName;

Wouldn't it also throw, per:
https://dxr.mozilla.org/mozilla-central/source/js/src/doc/Debugger/Debugger.Script.md#124
?
As well as startLine?
And other fields like strictMode and staticLevel which are not documented in the MD file.


Otherwise, I thought that this code wasn't used.
Did you got some devtools xpcshell or mochitest involving this codepath? I was about to remove this "hackDebugger" function as try is still green without it... so at very least we have no assertion covering this toString functions :/
Attachment #8830413 - Flags: review?(poirot.alex)
Comment on attachment 8830413 [details]
Bug 1333840 - Protect from frame.this access for wasm debugger frame.

https://reviewboard.mozilla.org/r/107188/#review108532

> Wouldn't it also throw, per:
> https://dxr.mozilla.org/mozilla-central/source/js/src/doc/Debugger/Debugger.Script.md#124
> ?
> As well as startLine?
> And other fields like strictMode and staticLevel which are not documented in the MD file.
> 
> 
> Otherwise, I thought that this code wasn't used.
> Did you got some devtools xpcshell or mochitest involving this codepath? I was about to remove this "hackDebugger" function as try is still green without it... so at very least we have no assertion covering this toString functions :/

The startLine and few other functions are implemented at bug 1330370 (I added a test, but it may pass with this bug patches)

You are right about displayName -- I just disable toString hack for wasm script. I cannot find anything about strictMode and staticLevel properties at Debugger API looking at the C++ code.
Depends on: 1330370
Comment on attachment 8830413 [details]
Bug 1333840 - Protect from frame.this access for wasm debugger frame.

https://reviewboard.mozilla.org/r/107188/#review108532

> The startLine and few other functions are implemented at bug 1330370 (I added a test, but it may pass with this bug patches)
> 
> You are right about displayName -- I just disable toString hack for wasm script. I cannot find anything about strictMode and staticLevel properties at Debugger API looking at the C++ code.

Tests are added and bug 1330370 is landing.
Comment on attachment 8830413 [details]
Bug 1333840 - Protect from frame.this access for wasm debugger frame.

https://reviewboard.mozilla.org/r/107188/#review109374

FYI, the stack doesn't seem to print the wasm frames in the new debugger (devtools.debugger.new-debugger-frontend=true),
whereas we see it in the old debugger UI (devtools.debugger.new-debugger-frontend=false)

The new debugger is preffed on in nightly and dev-edition.

::: devtools/server/tests/unit/test_frameactor_wasm-01.js:74
(Diff revision 3)
> +    ]));
> +    var i = new WebAssembly.Instance(m, {a: {b: () => {
> +      debugger;
> +    }}});
> +    i.exports.c();
> +    debugger;

I don't quite get why you need two call to `debugger;` and two "paused" listener?
Can't you just keep the debugger in b function and reset the pref and call finishClient after the location.source.url assert?
Attachment #8830413 - Flags: review?(poirot.alex) → review+
Comment on attachment 8830413 [details]
Bug 1333840 - Protect from frame.this access for wasm debugger frame.

https://reviewboard.mozilla.org/r/107188/#review109374

Shall be fixed at https://github.com/devtools-html/debugger.html/pull/1864 (but not sync'ed with m-c yet)
Pushed by ydelendik@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/592325b7b0d1
Protect from frame.this access for wasm debugger frame. r=ochameau
https://hg.mozilla.org/mozilla-central/rev/592325b7b0d1
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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: