Closed Bug 1013941 Opened 10 years ago Closed 5 years ago

Error after porting debugger frontend to Task.jsm: aItem is null

Categories

(DevTools :: Debugger, defect, P3)

Product:
DevTools
Component:
Debugger
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: jlong, Unassigned)

Details

Attachments

(1 file, 3 obsolete files)

Don't call ensureIndexIsVisible with an out of bounds argument, remove the unneeded dirty flag and fix a number of races in tests
11.10 KB, patch
Details | Diff | Splinter Review 
I'm not sure if this was caused by the Task.jsm port or not, but it's possible. I haven't debugged an error this deep in the stack before, so I'm unsure what's happening.

When running browser_dbg_variables-view-edit-getset-01.js, the test passes but this is in the console:

 2:55.07 ************************************************************
 2:55.07 * Call to xpconnect wrapped JSObject produced this error:  *
 2:55.07 [Exception... "[JavaScript Error: "aItem is null" {file: "resource:///modules/devtools/ViewHelpers.jsm" line: 804}]'[JavaScript Error: "aItem is null" {file: "resource:///modules/devtools/ViewHelpers.jsm" line: 804}]' when calling method: [nsITimerCallback::notify]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js :: EventLoop.prototype.enter :: line 434"  data: yes]
 2:55.07 ************************************************************
Depends on: 1013945
No longer depends on: 1013945
I don't seem to get this anymore with fx-team tip. Also ViewHelpers.jsm:804 is now something entirely different.
Blocks: 1011603
No longer depends on: 1011603
This doesn't get reported from Task.jsm, so no need to block bug 1011603.
No longer blocks: 1011603
Assignee: nobody → past
Status: NEW → ASSIGNED
Priority: -- → P3
The same error appears when running browser_dbg_variables-view-edit-getset-02.js, but fixing it for both exposes a race in the second test.
The original fix was rather straightforward, but I couldn't figure out how to fix the race in the second test until I rewrote it with Task.jsm.
Attachment #8462650 - Flags: review?(vporof)
And in the unlikely case that you are not a mind reader, there were two problems with the second test.

The first was that editing the value would throw in activate() with "input.select is not a function". I figured that the problem was that the node wasn't visible when the mousedown event was triggered, so I arranged for the variables view to scroll down first.

The second was that the FETCHED_WATCH_EXPRESSIONS event that appeared after the FETCHED_PROPERTIES event wasn't the one we were expecting, but was originating from a previous protocol request. This is what was allowing the test to finish before. Adding an extra waitForDebuggerEvents fixed that, too.
Comment on attachment 8462650 [details] [diff] [review]
Don't call ensureIndexIsVisible with an out of bounds argument and fix a race in browser_dbg_variables-view-edit-getset-02.js

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

::: browser/devtools/debugger/test/browser_dbg_variables-view-edit-getset-02.js
@@ +17,5 @@
> +    let gDebugger = panel.panelWin;
> +    let L10N = gDebugger.L10N;
> +    let editor = gDebugger.DebuggerView.editor;
> +    let gVars = gDebugger.DebuggerView.Variables;
> +    let gWatch = gDebugger.DebuggerView.WatchExpressions;

Nit: Since these vars are all local, and this file is mostly rewritten, I'd suggest losing the `g` prefix.
Attachment #8462650 - Flags: review?(vporof) → review+
The oranges in the previous try run uncovered another issue. We were often needlessly making a protocol request to get more stack frames, even if there were no more frames to be fetched (which we knew since the last frame had frame.oldest == true). The timing of that extra request would work almost always fine for optimied builds, but would almost always fail in debug builds.

That was the reason for the extra waitForDebuggerEvents that I had added in the previous version of the test. I took it out now that I've realized it was only papering over the actual problem.
Attachment #8463976 - Flags: review?(vporof)
Attachment #8462650 - Attachment is obsolete: true
Comment on attachment 8463976 [details] [diff] [review]
Don't call ensureIndexIsVisible with an out of bounds argument and fix a race in browser_dbg_variables-view-edit-getset-02.js v2

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

::: browser/devtools/debugger/debugger-toolbar.js
@@ +583,4 @@
>        this.dirty = false;
>  
>        // Loads more stack frames from the debugger server cache.
> +      if (DebuggerController.activeThread.moreFrames) {

Shouldn't this ever happen? We're setting the dirty flag in _refillFrames, in debugger-controller.js, which guards against _afterScroll being called.

r? me again after letting me know. Should we just remove the dirty flag?
Attachment #8463976 - Flags: review?(vporof)
So what happens *in the browser_dbg_variables-view-edit-getset-02.js test*, is that after adding the watch expression execution pauses because of the clientEvaluate packet. _onScroll is called with dirty == false. Regular pause handling requests the frames and clears the existing ones, which causes _onScroll to be called again with dirty == true and arranges for _afterScroll to be called after a timeout. Meanwhile, the response to the frames request arrives and sets dirty = false (since it just got all of the frames). Now it's time for _afterScroll to get called, which gets dirty == false (but we don't check for it) and proceeds to set dirty to false again and request the frames, etc.

When using the browser normally (i.e. outside of this test) I don't trigger _onScroll, so the above doesn't happen.

In any case it is reasonable to assume that since _afterScroll is called after a timeout, it should check the value of dirty again. I'm not sure if that should happen in the beginning or if we want to call ensureIndexIsVisible anyway. And to answer your last question, it doesn't look like dirty is buying us much over moreFrames, so I would be in favor of removing it entirely, to simplify the code a bit.
Comment on attachment 8463976 [details] [diff] [review]
Don't call ensureIndexIsVisible with an out of bounds argument and fix a race in browser_dbg_variables-view-edit-getset-02.js v2

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

Alright, let's just remove the dirty flag entirely.
Attachment #8463976 - Flags: review?(vporof) → review+
Do you think _afterScroll should bail early like _onScroll if !moreFrames or only skip addMoreFrames?
Yeah, I would remove the dirty check from _onScroll and do it first thing inside _afterScroll.
I think leaving the check in _onScroll is a nice little optimization, too. Since it can get called a lot, it would be good to avoid calling setTimeout unnecessarily.
Attachment #8463976 - Attachment is obsolete: true
Comment on attachment 8464617 [details] [diff] [review]
Don't call ensureIndexIsVisible with an out of bounds argument and fix a race in browser_dbg_variables-view-edit-getset-02.js v3

Carrying r+.
Attachment #8464617 - Flags: review+
Fixed an oversight in the replacement of "dirty" with "moreFrames" and fixed another race in tests. Let's see how green this one is.
Attachment #8464617 - Attachment is obsolete: true
I don't have the time to look into the test failures from the latest patch iteration.
Assignee: past → nobody
Status: ASSIGNED → NEW
Product: Firefox → DevTools

closing as this test is no longer around browser_dbg_variables-view-edit-getset

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: