1113865 - Replace all calls to dbg_assert with DevToolsUtils.assert

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

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

Right now it doesn't, even when overridden to have fatal behavior.

Comment 1

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

Attachment: dbg-assert.patch

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=60303f6d2c04

Comment 2

[James Long (:jlongster)]

You know, I just noticed this while fixing bug 1111058; the `source` method had one of these and it wasn't failing!

Comment 3

[James Long (:jlongster)]

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 4

[James Long (:jlongster)]

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...

Comment 5

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

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.

Comment 6

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

It was a return on purpose though, not a throw, so that it wasn't fatal in the non-"debug" modes.

Comment 7

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

Attachment: dbg-assert.patch

Ok, this consolidates the fatal behavior into something akin to how we do logging.

Comment 8

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

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

Comment 9

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

Attachment: dbg-assert.patch

This fixes a reference error typo from the last version of the patch.

Comment 10

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

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.

Comment 11

[James Long (:jlongster)]

(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 12

[James Long (:jlongster)]

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.

Comment 13

[James Long (:jlongster)]

Yeah, I'll add this as a Q4 goal.

Comment 14

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

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.

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

This is loud.

Comment 17

[Brian Grinstead [:bgrins]]

(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.

Comment 18

[Brian Grinstead [:bgrins]]

Seems like this should land in the same release as Bug 1214775.  James, are you still planning to work on this?

Comment 19

[James Long (:jlongster)]

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.

Comment 20

[Tobias B. Besemer [:BesTo] (QA)]

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

Comment 21

[Tobias B. Besemer [:BesTo] (QA)]

(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 ... ;-)

Comment 22

[Alexandre Poirot [:ochameau]]

These stacks kill me.

Comment 23

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49680e09f0f7

Comment 24

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

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

Comment 25

[Alexandre Poirot [:ochameau]]

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

Comment 26

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dcb1ff466fb

Comment 27

[Alexandre Poirot [:ochameau]]

Attachment: 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!

Comment 28

[Alexandre Poirot [:ochameau]]

Attachment: 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

Comment 29

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

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?

Comment 30

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

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!

Comment 31

[Alexandre Poirot [:ochameau]]

(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.

Comment 32

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

(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.

Comment 33

[Brian Grinstead [:bgrins]]

(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).

Comment 34

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4d6cfb9064c

Comment 35

[Alexandre Poirot [:ochameau]]

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

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e2b7001ebc4