Closed Bug 1319365 Opened 3 years ago Closed 3 years ago

browser_rules_edit-selector-click.js has an async error of this.inspector is null that is reported after leaving the test bound

Categories

(DevTools :: Inspector: Rules, defect, P3)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: gl, Assigned: gl)

Details

Attachments

(1 file, 1 obsolete file)

STR:
Run ./mach mochitest devtools/client/inspector/rules/test/browser_rules_edit-selector-click.js

You will see after the leaving the test bound that there is an error:
43 INFO Leaving test bound
*************************
A coding exception was thrown and uncaught in a Task.
Full message: TypeError: this.inspector is null
Full stack: CssRuleView.prototype.highlightSelector<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/rules/rules.js:273:9
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:311:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
CssRuleView.prototype.toggleSelectorHighlighter/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/rules/rules.js:262:9
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:932:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:452:5
Inspector.prototype.destroy@chrome://devtools/content/inspector/inspector.js:897:34
Toolbox.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:2253:26
DT_closeToolbox@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/devtools.js:469:12
closeTabAndToolbox<@chrome://mochitests/content/browser/devtools/client/framework/test/shared-head.js:367:11
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:311:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
cleanup@chrome://mochitests/content/browser/devtools/client/framework/test/shared-head.js:104:11
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
TaskImpl@resource://gre/modules/Task.jsm:277:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:252:14
Task_spawn@resource://gre/modules/Task.jsm:166:12
TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:389:16
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:401:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:401:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:401:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
TaskImpl@resource://gre/modules/Task.jsm:277:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:252:14
testScope/test_finish/<@chrome://mochikit/content/browser-test.js:1008:11
testScope/test_executeSoon/<.run@chrome://mochikit/content/browser-test.js:939:9

I believe this is happening because rule-editor.js#_onSelectorDone queues up rules.js#toggleSelectorHighlighter which runs the async functions highlighterSelector and unhighlightSelector and consequently gets called after the toolbox/inspector is destroyed in this test. So, we need to check for this.inspector and bail out if it doesn't exist.
Attached patch 1319365.patch (obsolete) — Splinter Review
Attachment #8813082 - Flags: review?(jdescottes)
Comment on attachment 8813082 [details] [diff] [review]
1319365.patch

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

Looks good to me, thanks.

FWIW, both methods bail out if the highlighter can't be retrieved. We could alternatively reuse that, update getSelectorHighlighter to return null when this.inspector is unavailable. But I'm fine with the current fix as well.
Attachment #8813082 - Flags: review?(jdescottes) → review+
Attachment #8813082 - Attachment is obsolete: true
Attachment #8813117 - Flags: review+
Switched to bailing out in getSelectorHighlighter
https://hg.mozilla.org/integration/mozilla-inbound/rev/39efc7bfb839484ef121a2f275481770906d9021
Bug 1319365 - Bail out of the async function getSelectorHighlighter when the inspector is unavailable. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/39efc7bfb839
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.