Closed Bug 1547320 Opened 2 years ago Closed 2 years ago

Can't debug typed arrays with custom prototype in a worker

Categories

(DevTools :: Debugger, defect, P3)

67 Branch
defect

Tracking

(firefox-esr60 unaffected, firefox-esr68 wontfix, firefox66 unaffected, firefox67 wontfix, firefox68 wontfix, firefox69 fixed, firefox70 fixed)

RESOLVED FIXED
Firefox 70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox66 --- unaffected
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- fixed
firefox70 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Bug 1522244 broke the getArrayLength function which is used to safely obtain the length of an array. Then debugging typed arrays is not reliable in workers.

  1. Go to about:blank
  2. Open the debugger
  3. Run this in the console:
    new Worker(encodeURI(`data:application/javascript,
      var arr = new Uint8Array([1,2,3]);
      Object.setPrototypeOf(arr, null);
      debugger;
    `))
    

Result:

ObjectActor.prototype.grip previewer function threw an exception: TypeError: object.proto is null
Stack: getArrayLength@resource://devtools/server/actors/object/utils.js:193:20
form@resource://devtools/server/actors/object.js:138:29
objectGrip@resource://devtools/server/actors/thread.js:1398:18

Brian you up for looking at this when you have a chance?

Flags: needinfo?(bhackett1024)

Oh, now I understand why bug 1522244 added that, it's not not as easy as I thought. test_objectgrips-20.js passes for workers with my patch, but steps in comment 0 still don't work...

Before bug 1522244, typed array previewing did not work at all in workers; fixing that and other worker previewing problems was, after all, the point of doing bug 1522244. I think there are multiple issues preventing the devtools server code in the worker from being able to wrap its state into the debuggee and call its own typed array length getter, and I'm not sure how easy they are to fix. For bug 1522244 I punted on this, both because doing so was easiest and because philosophically I feel that calling .length on the typed array in the debuggee is the right thing to do. If users want to modify how the length of a typed array is computed then the debugger should respect that, with the possibility that it might prevent the typed array from being fully inspectable by the debugger (the debugger should not break entirely, though). I don't feel strongly about this, and there is definitely a cognitive dissonance between how issues like this are handled in the main thread vs. workers due to bug 1522244 and bug 1533202, which would be nice to sort out.

Flags: needinfo?(bhackett1024)

It's a bit strange, though, that test_objectgrips-20.js passes for workers when removing the code from bug 1522244. But in practice it doesn't work, calling the getter throws.

Interestingly, doing

return new globalThis[object.class](object.unsafeDereference()).length;

seems to work fine, the downside is that the data is copied.

So in fact I think that this is a JS problem, all steps performed by get %TypedArray%.prototype.length should also happen in TypedArray(typedArray), so it doesn't make sense that the former throws and the latter does not.

It doesn't help that the error is an Opaque object, so I can't read the message. And probably the code runs in an invisibleToDebugger compartment, I can't debug it. I guess I should debug the C++ instead...

(In reply to Brian Hackett (:bhackett) from comment #4)

philosophically I feel that calling .length on the typed array in the debuggee is the right thing to do.

I disagree (bug 1406182). If you have var arr = Object.setPrototypeOf(new Uint8Array([1,2,3]), null), there is no arr.length, but you can still use arr[0], arr[1], etc. to access the data. So the devtools shouldn't hide this data.

That said, if the [[ArrayLength]] cannot be easily accessed in workers, then using object.proto.proto.getOwnPropertyDescriptor("length").get instead doesn't make sense because it makes various unreliable assumptions. So I would agree to change it to the simpler DevToolsUtils.getProperty(object, "length") >>> 0 (only for workers) until a reliable solution is found.

At least that would work for new (class extends Int8Array {})([1, 2]), which currently fails due to the extra object in the prototype chain.

67 is close to flipping to release. Will this fix be ready (and safe) for a beta uplift?

Flags: needinfo?(oriol-bugzilla)

So, the thing is that I'm not much sure about what to do:

  • Fix JS so that Uint8Array.prototype.length can be called on the object in workers. I don't know much about this so unless someone else wants to take the bug, it won't be ready for beta.
  • Use new globalThis[object.class](object.unsafeDereference()).length in workers. The downside is that it copies the buffer. This is easy and it seems to work well, but maybe in some case it could still fail like calling the getter.
  • Use DevToolsUtils.getProperty(object, "length") in workers. Unreliable, but probably better than the current approach. Easy to do.

Brian, what do you prefer?

Flags: needinfo?(oriol-bugzilla) → needinfo?(bhackett1024)

Too late for a beta uplift.

Blocks: dbg-worker
Priority: -- → P3

(In reply to Oriol Brufau [:Oriol] from comment #7)

So, the thing is that I'm not much sure about what to do:

  • Fix JS so that Uint8Array.prototype.length can be called on the object in workers. I don't know much about this so unless someone else wants to take the bug, it won't be ready for beta.
  • Use new globalThis[object.class](object.unsafeDereference()).length in workers. The downside is that it copies the buffer. This is easy and it seems to work well, but maybe in some case it could still fail like calling the getter.
  • Use DevToolsUtils.getProperty(object, "length") in workers. Unreliable, but probably better than the current approach. Easy to do.

Brian, what do you prefer?

I don't think #2 should be done, as copying large array buffers unnecessarily could break the debugger entirely. #1 and #3 are both fine, but it would be nice to fix the inconsistent behavior between workers and the main thread, which would mean doing either #1 using #3 in both cases.

Flags: needinfo?(bhackett1024)

I debugged the Uint8Array.prototype.length way a bit, the call stack is

When not in a worker, this continues with

Then, back in Compartment::getOrCreateWrapper, wrapper(cx, wrap(cx, existing, obj)) is truthy.

However, for workers, it goes like this:

It has this check:

  if (!IsWorkerDebuggerGlobal(targetGlobal) &&
      !IsWorkerDebuggerSandbox(targetGlobal)) {
    JS_ReportErrorASCII(
        cx, "There should be no edges from the debuggee to the debugger.");
    return nullptr;
  }

If I remove it, the Uint8Array.prototype.length getter succeeds. So I wonder if it can just be removed or if that would cause some problems.

BTW, in that same function there is also

  if (IsWorkerDebuggerGlobal(originGlobal) ||
      IsWorkerDebuggerSandbox(originGlobal)) {
    wrapper = &js::CrossCompartmentWrapper::singleton;
  } else {
    wrapper = &js::OpaqueCrossCompartmentWrapper::singleton;
  }

Using OpaqueCrossCompartmentWrapper means that the error thrown when calling the getter is an opaque object, so the message "There should be no edges from the debuggee to the debugger." can't be read. That's not useful.

Who can I ask about these? Eddy Bruel and Kyle Huey don't seem active anymore.

Jim, could DebuggerObject expose native, original, unmodified functions of the compartment of the object? Like %TypedArray%.prototype.length but also Map.prototype.keys for https://searchfox.org/mozilla-central/rev/cc280c4be94ff8cf64a27cc9b3d6831ffa49fa45/devtools/server/actors/object/property-iterator.js#258. Then it wouldn't be cross-compartment so it should work.

Flags: needinfo?(jimb)

Bulk change to wontfix for 68 (P3+ carryover with needinfo).

(In reply to Oriol Brufau [:Oriol] from comment #10)

  if (!IsWorkerDebuggerGlobal(targetGlobal) &&
      !IsWorkerDebuggerSandbox(targetGlobal)) {
    JS_ReportErrorASCII(
        cx, "There should be no edges from the debuggee to the debugger.");
    return nullptr;
  }

If I remove it, the Uint8Array.prototype.length getter succeeds. So I wonder if it can just be removed or if that would cause some problems.

I would try getting this block of code removed. The fact this works on the main thread but not in workers suggests that the worker is being unnecessarily conservative when wrapping.

Who can I ask about these? Eddy Bruel and Kyle Huey don't seem active anymore.

Looking at recent reviewers of RuntimeService.cpp, I think you could ask asuth or smaug.

Bug 1325195 removes the wrapper error and all the worker specific logic in the previewers, which should fix this bug provided it is actually able to land.

Attachment #9061034 - Attachment is obsolete: true
Depends on: 1325195
Keywords: checkin-needed

Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/acb35a64356b
Test that devtools can safely obtain the length of typed arrays in workers. r=bhackett
https://hg.mozilla.org/integration/autoland/rev/f48869faad2e
Fix formatting of test_objectgrips-20.js. r=bhackett

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

The problem was fixed in bug 1325195 for firefox69. Here I only added some tests.

Flags: in-testsuite+

[Cleaning up old needinfos]

(In reply to Oriol Brufau [:Oriol] from comment #11)

Jim, could DebuggerObject expose native, original, unmodified functions of the compartment of the object? Like %TypedArray%.prototype.length but also Map.prototype.keys for https://searchfox.org/mozilla-central/rev/cc280c4be94ff8cf64a27cc9b3d6831ffa49fa45/devtools/server/actors/object/property-iterator.js#258. Then it wouldn't be cross-compartment so it should work.

I think the only information we can trust about a debuggee object is its JSClass. If a class has a ClassSpec, then I imagine we could trawl through that and construct fresh native functions in the referent's compartment that debugger code could invoke.

This would help in some cases, but the Debugger will often need inspection powers beyond those available to content - say, to inspect a weak map's entries. I proposed some infrastructure to make it easier to add such powers in bug 1257665.

Flags: needinfo?(jimb)
You need to log in before you can comment on or make changes to this bug.