Closed Bug 1583151 Opened 11 months ago Closed 10 months ago

Log important errors to stdout (follow up to Bug 1581232)

Categories

(DevTools :: Debugger, task)

task
Not set
normal

Tracking

(firefox71 fixed)

RESOLVED FIXED
Firefox 71
Tracking Status
firefox71 --- fixed

People

(Reporter: jdescottes, Assigned: jlast)

References

Details

Attachments

(1 file)

There are 2 issues with the patch that landed in Bug 1581232:

1/
A comment was not addressed before landing: https://phabricator.services.mozilla.com/D46533#1410184

Can you also update the dumpn call in reportError() in thread.js? That is the one that motivated filing this bug.

So we are still not logging the error that made the investigation in Bug 1577881 difficult.

2/
dumpn adds a "\n" at the end of the message:

exports.dumpn = function(str) {
  if (flags.wantLogging) {
    dump("DBG-SERVER: " + str + "\n");
  }
};

With this patch, all errors will be concatenated with the next line, which is not ideal.
We are also missing the prefix "DBG-SERVER" which could be useful.

I initially wanted to REOPEN Bug 1581232, but it just makes patches hard to track, so I'm just doing a follow up.
Jason, you landed the patch from Bug 1581232, could you take care of this?

Flags: needinfo?(jlaster)

Also. Even if the patch undercovers an exception in bug 1583447, I'm wondering if we want to print all uncaught exceptions of the debuggees?
It means that, even on production, if the content page is having unchaught exception, it will print them on stdout.
I would rather expect that to be done by the test harness and may be only do that in tests.
SimpleTest is actually having some code to do that here, but that only catches exceptions thrown in the context of the mochitest browser script. That explains why it wasn't reporting the exception happening from a DevTools sandbox, as it wasn't called by the test script.

Otherwise, even if we want to keep that report. It may be better to stop setting an uncaughtException handler from the thread actor, from here and instead let the one already registered by make-debugger, which seems to be already doing a better job with \n see that code.

(In reply to Alexandre Poirot [:ochameau] from comment #2)

Also. Even if the patch undercovers an exception in bug 1583447, I'm wondering if we want to print all uncaught exceptions of the debuggees?
It means that, even on production, if the content page is having unchaught exception, it will print them on stdout.
I would rather expect that to be done by the test harness and may be only do that in tests.
SimpleTest is actually having some code to do that here, but that only catches exceptions thrown in the context of the mochitest browser script. That explains why it wasn't reporting the exception happening from a DevTools sandbox, as it wasn't called by the test script.

^-- This part of my comment is wrong. uncaughtExceptionHook is only called when a breakpoint, watchpoint or similar is throwing.
I got confused as I was not expecting any breakpoint or watchpoint to be executed during a browser console opening. So I think it is the right thing to do here.

The second part of my comment, is, I think, still relevant.

Pushed by jlaster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d45644f68888
Log important errors to stdout (follow up). r=jdescottes
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b5b267397629
Follow up to jlaster's push. r=jlast. CLOSED TREE
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71
Assignee: nobody → jlaster
Flags: needinfo?(jlaster)
You need to log in before you can comment on or make changes to this bug.