Replace all calls to dbg_assert with DevToolsUtils.assert

RESOLVED FIXED in Firefox 45

Status

()

Firefox
Developer Tools: Debugger
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: fitzgen, Assigned: ochameau)

Tracking

unspecified
Firefox 45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Right now it doesn't, even when overridden to have fatal behavior.
Created attachment 8539539 [details] [diff] [review]
dbg-assert.patch

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=60303f6d2c04
Attachment #8539539 - Flags: review?(jlong)
You know, I just noticed this while fixing bug 1111058; the `source` method had one of these and it wasn't failing!
Comment on attachment 8539539 [details] [diff] [review]
dbg-assert.patch

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

::: browser/devtools/debugger/test/head.js
@@ +61,5 @@
> +function dbg_assert(condition, message) {
> +  if (!condition) {
> +    const err = new Error(message);
> +    DevToolsUtils.reportException("dbg_assert failed", err);
> +    throw err;

The additional `reportException` looks good to me, but can you explain to my untrained eye why it's needed? Why doesn't just the `throw` work? Same with the other tweaks.

Also, is there any way we could consolidate these definitions? Could all the head* files import dbg_assert fomr DevToolsUtils?

::: toolkit/devtools/DevToolsUtils.js
@@ -338,2 @@
>    if (!cond) {
> -    return e;

This `return` here is bizarre, seems like a bug! Shouldn't it be `throw`?

This is the old code though; in your new code you don't have the `return`. Should it also throw though?
Comment on attachment 8539539 [details] [diff] [review]
dbg-assert.patch

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

::: toolkit/devtools/DevToolsUtils.js
@@ -338,2 @@
>    if (!cond) {
> -    return e;

This comment may have been confusing; I'm looking at the old `dbg_assert` in DevToolsUtils and it's weird to me that it used to just return...
Yeah the return was weird and unused, so I removed it.

Only doing a throw doesn't work because it could either (a) be caught and ignored, which isn't in the spirit of assertions, or (b) unwind the stack and then get lost due to the crappy error reporting we have (which is why we have reportException in the first place).

The reason I don't throw in the non-fatal version is because an assertion should be almost a noop in non-"debug" modes/builds/whatever (we kind of have our own devtools debug mode).

I can definitely add a comment explaining what's going on and consolidate the fatal versions.

ni? me so I don't forget once I'm back after the holidays.
Flags: needinfo?(nfitzgerald)
It was a return on purpose though, not a throw, so that it wasn't fatal in the non-"debug" modes.
Created attachment 8543064 [details] [diff] [review]
dbg-assert.patch

Ok, this consolidates the fatal behavior into something akin to how we do logging.
Attachment #8539539 - Attachment is obsolete: true
Attachment #8539539 - Flags: review?(jlong)
Flags: needinfo?(nfitzgerald)
Attachment #8543064 - Flags: review?(jlong)
Aaaaaaaaaaaand this uncovers existing bugs that happen in our existing tests:

    PROCESS | 64198 | dbgAssert failed threw an exception: Error: ThreadSources.prototype.source needs an originalUrl or a source
    PROCESS | 64198 | Stack: exports.dbgAssert@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:350:17
    PROCESS | 64198 | ThreadSources.prototype.source@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:5150:1
    PROCESS | 64198 | ThreadSources.prototype.getOriginalLocation/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:5542:37
    PROCESS | 64198 | Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:870:23
    PROCESS | 64198 | this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:749:7
    PROCESS | 64198 | this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:691:37
    PROCESS | 64198 | EventLoop.prototype.enter@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:363:5
    PROCESS | 64198 | ThreadActor.prototype._pushThreadPause@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:572:5
    PROCESS | 64198 | ThreadActor.prototype._pauseAndRespond@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:771:7
    PROCESS | 64198 | ThreadActor.prototype.onDebuggerStatement@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:1898:9
    PROCESS | 64198 | @/Users/fitzgen/src/mozilla-central-opt/obj.noindex/_tests/xpcshell/toolkit/devtools/server/tests/unit/test_sourcemaps-03.js line 85 > eval:2:5
    PROCESS | 64198 | @/Users/fitzgen/src/mozilla-central-opt/obj.noindex/_tests/xpcshell/toolkit/devtools/server/tests/unit/test_sourcemaps-03.js line 85 > eval:1:2
    PROCESS | 64198 | testBreakpointMapping@/Users/fitzgen/src/mozilla-central-opt/obj.noindex/_tests/xpcshell/toolkit/devtools/server/tests/unit/test_sourcemaps-03.js:85:1
    PROCESS | 64198 | _onNewSource@/Users/fitzgen/src/mozilla-central-opt/obj.noindex/_tests/xpcshell/toolkit/devtools/server/tests/unit/test_sourcemaps-03.js:105:5
    PROCESS | 64198 | eventSource/aProto.emit@resource://gre/modules/devtools/dbg-client.jsm:190:9
    PROCESS | 64198 | DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:942:7
    PROCESS | 64198 | LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:545:11
    PROCESS | 64198 | makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
    PROCESS | 64198 | makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
    PROCESS | 64198 | _do_main@/Users/fitzgen/src/mozilla-central-opt/testing/xpcshell/head.js:184:5
    PROCESS | 64198 | _execute_test@/Users/fitzgen/src/mozilla-central-opt/testing/xpcshell/head.js:476:5
    PROCESS | 64198 | @-e:1:1
    PROCESS | 64198 | Line: 350, column: 16
Created attachment 8543090 [details] [diff] [review]
dbg-assert.patch

This fixes a reference error typo from the last version of the patch.
Attachment #8543064 - Attachment is obsolete: true
Attachment #8543064 - Flags: review?(jlong)
Attachment #8543090 - Flags: review?(jlong)
Will look into fixing the existing but uncovered failures soon. (Certainly before this lands).

Might ping you for help James, because that one at least looks possibly related to recent source refactorings.
(In reply to Nick Fitzgerald [:fitzgen] from comment #8)
> Aaaaaaaaaaaand this uncovers existing bugs that happen in our existing tests:

I'm 97% sure that bug 1111058 will fix that. I noticed while working on the bug that that dbg_assert should have failed and was wondering why it didn't. It should be refactored to adhere to that dbg_assert.

This patch is looking good, I'll review it in detail soon (catching up on a few other things first)
Comment on attachment 8543090 [details] [diff] [review]
dbg-assert.patch

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

This looks very nice! I tried to apply it to bug 1111058 but there's a small conflict, so I haven't tested if that bug fixes it or not yet. That bug should land tomorrow (barring test failures) so we can rebase and see if it fixes the test failures this brought up.
Attachment #8543090 - Flags: review?(jlong) → review+
Assignee: nfitzgerald → jlong
Yeah, I'll add this as a Q4 goal.
Depends on: 1214775
Note that I'm landing a better asserter in bug 1214775 and deprecating dbg_assert at the same time. Leaving this bug for fixing the existing failing dbg_assert() calls and porting over to DevToolsUtils.assert.
Summary: Make dbg_assert actually report assertion failures → Replace all calls to dbg_assert with DevToolsUtils.assert
Duplicate of this bug: 1216215
This is loud.
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #16)
> This is loud.

Yeah.  Looks like there's an r+ed patch on this bug but I guess that's for something else as per Comment 14.   If we can at least get rid of the warning that shows up when the toolbox opens that would remove of a lot of logspam.
Seems like this should land in the same release as Bug 1214775.  James, are you still planning to work on this?
status-firefox44: --- → affected
Flags: needinfo?(jlong)
No way I'll get this done by the Nov 2 uplift, already booked for the next 2 weeks. I think it would be wise to remove the deprecation warning until after the uplift.

This is a Q4 goal of mine, and I can certainly get it done in Q4, but not by Nov 2.
Assignee: jlong → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(jlong)
Depends on: 1218005
Guess this is this error I get in the Browser Console, right?

"DevToolsUtils.dbg_assert is deprecated! Use DevToolsUtils.assert instead!
dbg_assert@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:449:13
ObjectActor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/object.js:57:1
WCA_objectGrip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:451:17
createValueGrip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/object.js:1913:1
WCA_createValueGrip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:410:12
WCA_prepareConsoleMessageForRemote/result.arguments<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:1547:14
WCA_prepareConsoleMessageForRemote@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:1545:24
WCA_onGetCachedMessages/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:739:27
WCA_onGetCachedMessages@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:738:11
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1599:15
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:569:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:87:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:87:14
" DevToolsUtils.js:452


And this one, too?

Property contained reference to invalid variable.  Error in parsing value for 'color'.  Falling back to 'inherit'. devedition.css:206:7206
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Tobias B. Besemer [:BesTo] (QA) from comment #20)
> And this one, too?
> 
> Property contained reference to invalid variable.  Error in parsing value
> for 'color'.  Falling back to 'inherit'. devedition.css:206:7206

Guess not, but maybe someone can fix it without a separate bug report ... ;-)
(Assignee)

Comment 22

2 years ago
These stacks kill me.
Assignee: nobody → poirot.alex
(Assignee)

Comment 23

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49680e09f0f7
(Assignee)

Comment 24

2 years ago
Created attachment 8683350 [details] [diff] [review]
patch v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=714176d866ff
(Assignee)

Updated

2 years ago
Attachment #8543090 - Attachment is obsolete: true
(Assignee)

Comment 25

2 years ago
In this patch, I had to disable assert() in workers as there is no Cu available in order to access to AppConstants.
(Assignee)

Comment 26

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dcb1ff466fb
(Assignee)

Comment 27

2 years ago
Created attachment 8683639 [details] [diff] [review]
Replace dbg_assert by assert v2

Splitting the patch in two.
This one just replace dbg_assert by assert everywhere *except*:
devtools/server/actors/script.js:    dbg_assert(this._thread.state === "running", "Should be in the running state");
Which happen to fail for: devtools/client/debugger/test/mochitest/browser_dbg_break-on-dom-08.js
And I haven't been able to figure it out in reasonable time,
so I'm leaving this one up for experts!

At least, we won't have the annoying dbg_assert deprecation warning from object.js!
Attachment #8683639 - Flags: review?(nfitzgerald)
(Assignee)

Updated

2 years ago
Attachment #8683350 - Attachment is obsolete: true
(Assignee)

Comment 28

2 years ago
Created attachment 8683641 [details] [diff] [review]
green try tweaks - v1

And the various things required to have a green try:
- isWorker check added to allow calling assert() from workers
  (it disables assert() from worker for DEBUG/DEBUG_JS_MODULES cases)
  (also, I don't think we toggle DevToolsUtils.testing on workers?)
- attachTab added to various promises test,
  required to prevent:
  http://mxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#897
- fake a source object in testing/xpcshell/head.js
  to prevent: http://mxr.mozilla.org/mozilla-central/source/devtools/server/actors/utils/TabSources.js#106
  when running: http://mxr.mozilla.org/mozilla-central/source/devtools/server/tests/unit/test_xpcshell_debugging.js
- revert back to dbg_assert for devtools/server/actors/script.js
  as explained in previous comment
Attachment #8683641 - Flags: review?(nfitzgerald)
Comment on attachment 8683641 [details] [diff] [review]
green try tweaks - v1

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

Thanks for doing this!

::: devtools/shared/DevToolsUtils.js
@@ +460,5 @@
>  
>  exports.defineLazyGetter(this, "AppConstants", () => {
> +  if (isWorker) {
> +    return {};
> +  }

It is sad that this is necessary. Can you file a follow up to make AppConstants available in some form to workers / without Cu?
Attachment #8683641 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8683639 [details] [diff] [review]
Replace dbg_assert by assert v2

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

::: devtools/shared/transport/tests/unit/head_dbg.js
@@ +51,5 @@
>      kind = "strict " + kind;
>  
>    return kind;
>  }
>  

Can you double check that we set DevToolsUtils.testing to true in this head file? Same for the one below. Thanks!
Attachment #8683639 - Flags: review?(nfitzgerald) → review+
(Assignee)

Comment 31

2 years ago
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #30) 
> ::: devtools/shared/transport/tests/unit/head_dbg.js
>
> Can you double check that we set DevToolsUtils.testing to true in this head
> file? Same for the one below. Thanks!

Looks like we aren't. While throwing patches to try I've seen that various tests only fails on debug builds thanks to the constant.

I didn't knew DevtoolsUtils.testing was set like this, this is quite weak.
(In reply to Alexandre Poirot [:ochameau] from comment #31)
> (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #30) 
> > ::: devtools/shared/transport/tests/unit/head_dbg.js
> >
> > Can you double check that we set DevToolsUtils.testing to true in this head
> > file? Same for the one below. Thanks!
> 
> Looks like we aren't. While throwing patches to try I've seen that various
> tests only fails on debug builds thanks to the constant.
> 
> I didn't knew DevtoolsUtils.testing was set like this, this is quite weak.

Ideally, we would put it in our shared head.js file (devtools/client/framework/test/shared-head.js) and every testing head file would import that. It looks like it is there (https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/shared-head.js#45) so we would just need to import it here. That might result in some const-redefinition errors or something else due to shared lexical scope, so it may be easier to just set DevToolsUtils.testing true.

Agreed that it is quite weak.
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #32)
> (In reply to Alexandre Poirot [:ochameau] from comment #31)
> > (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #30) 
> > > ::: devtools/shared/transport/tests/unit/head_dbg.js
> > >
> > > Can you double check that we set DevToolsUtils.testing to true in this head
> > > file? Same for the one below. Thanks!
> > 
> > Looks like we aren't. While throwing patches to try I've seen that various
> > tests only fails on debug builds thanks to the constant.
> > 
> > I didn't knew DevtoolsUtils.testing was set like this, this is quite weak.
> 
> Ideally, we would put it in our shared head.js file
> (devtools/client/framework/test/shared-head.js) and every testing head file
> would import that. It looks like it is there
> (https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/
> test/shared-head.js#45) so we would just need to import it here. That might
> result in some const-redefinition errors or something else due to shared
> lexical scope, so it may be easier to just set DevToolsUtils.testing true.
> 
> Agreed that it is quite weak.

I have bugs on file to migrate each directory to shared-head blocking Bug 1181833.  Usually it's relatively easy to migrate (removing duplicate consts and functions) although sometimes like in the debugger it needs some extra help (I have a patch up for review in Bug 1181838 that migrates the debugger head.js file).
(Assignee)

Comment 34

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4d6cfb9064c
(Assignee)

Updated

2 years ago
Depends on: 1171304
(Assignee)

Comment 35

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/8e2b7001ebc461585bb003794e31e3f203a394c5
Bug 1113865 - Replace all calls to dbg_assert with DevToolsUtils.assert. r=fitzgen

Comment 36

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8e2b7001ebc4
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
You need to log in before you can comment on or make changes to this bug.