1038117 - "item is undefined" error at debugger-panes.js:163

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Visit http://htmlpad.org/debugger/ and open the debugger. The terminal contains this exception:

Handler function DebuggerClient.requester request callback threw an exception: TypeError: item is undefined
Stack: SourcesView.prototype<.addSource@chrome://browser/content/devtools/debugger-panes.js:163:7
SourceScripts.prototype._onSourcesAdded@chrome://browser/content/devtools/debugger-controller.js:1208:9
DebuggerClient.requester/</<@resource://gre/modules/devtools/dbg-client.jsm:344:9
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:96:9
Request.prototype.emit@resource://gre/modules/devtools/dbg-client.jsm:1082:29
DebuggerClient.prototype.onPacket/<@resource://gre/modules/devtools/dbg-client.jsm:922:9
resolve@resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
then@resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
then@resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9
DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:861:1
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:545:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
EventLoop.prototype.enter@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:432:5
ThreadActor.prototype._pushThreadPause@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:602:5
ThreadActor.prototype.onAttach@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:814:7
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1229:15
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:545:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
Line: 163, column: 6

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Only black box sources non-staged for later addition

We shouldn't be trying to black box sources staged for later addition.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Only black box sources non-staged for later addition

Actually, this is a better fix.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8455287 [details] [diff] [review]
Only black box sources non-staged for later addition

Review of attachment 8455287 [details] [diff] [review]:
-----------------------------------------------------------------

This is great if it works, but IIRC the contents element is not the same as the target element (target element is the contents' parent, I think) and we were having issues previously when applying the class to the contents element instead of the target element.

Hopefully, I'm misunderstanding!

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Black box sources before deciding whether to stage them for later addition v3

There is a test that was failing that I had to fix. Apart from a typo, the main problem is that the test expects to find the class set after a reload, but the black-box class gets set after an undetermined amount of time later. I couldn't find any usable events that would work for this test (there is no unblackboxed source present in the page), so I modified the check to test that the correct class gets set when the source is updated without staging instead. For the purposes of this test it should be equivalent.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Ah, mid-air collision! Were the issues you remember test failures or was the source label not getting grayed out? Both work fine for me.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8455364 [details] [diff] [review]
Black box sources before deciding whether to stage them for later addition v3

Review of attachment 8455364 [details] [diff] [review]:
-----------------------------------------------------------------

If everything works, then great :) However, I just did verify that the contents element is a child of the target element, so could you go through the existing code and make everything else use the contents element as well, so we don't sometimes add it to contents and othertimes add it to target?

http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#1244,1246

::: browser/devtools/debugger/test/browser_dbg_blackboxing-01.js
@@ +34,5 @@
>      ok(aSource.isBlackBoxed, "The source should be black boxed now.");
>      ok(bbButton.checked, "The checkbox should no longer be checked.");
> +    const selectedSource = getSelectedSourceElement(gPanel);
> +    ok(selectedSource.classList.contains("black-boxed"),
> +      "'black-boxed' class should still be applied");

We can't do this check here, we *have* to do it after the reload. The reason this check was added in bug 967156 was as a regression test because the styles weren't applied /after/ reload.

You can just call commit() on the Sources widget before grabbing the item, to make sure it isn't in staging limbo. I was thinking you could do getItemByValue too, but I don't think that checks staged items.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Black box sources before deciding whether to stage them for later addition v4

All comments addressed. I was wrong on IRC about calling commit() in the test not working. The problem was not commit(), but the fact that I was checking the wrong node for the class name.

I've added a getter for prebuiltNode for such non-private uses, just like we do for target, although I'd be in favor of just marking the properties non-private in such cases.

Try: https://tbpl.mozilla.org/?tree=Try&rev=25caeb24a94c

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/0139b24156be

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0139b24156be