Closed Bug 956804 Opened 9 years ago Closed 9 years ago

Use DevToolsUtils.reportException in Parser.jsm

Categories

(DevTools :: Debugger, defect, P3)

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 29

People

(Reporter: fitzgen, Assigned: alexey.novak.mail)

References

Details

Attachments

(3 files, 5 obsolete files)

mochitest_error.txt
11.41 KB, text/plain
Details
mozconfig.txt
717 bytes, text/plain
Details
956804.6.patch
5.69 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review 
We shouldn't reinvent the wheel: we have safe error stringification and reporting in DevToolsUtils.jsm (as |DevToolsUtils.reportException|) and we should use it in Parser.jsm instead of |log|.
If |log| is giving more meaningful output than |safeErrorString| via |reportException|, then we should just add that info to |safeErrorString|.
To whoever picks up this bug, we should remove the |log| function in browser/devtools/shared/Parser.jsm and instead import toolkit/devtools/DevToolsUtils.jsm and replace every call to |log| with a call to |DevToolsUtils.reportException| instead.

To get up and running: https://wiki.mozilla.org/DevTools/Hacking

Parser.jsm's |log| function definition: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/Parser.jsm#2352

DevToolsUtils.jsm's |reportException| definition: https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/DevToolsUtils.js#41

Import |DevToolsUtils| with this snippet:

> const { DevToolsUtils } = Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm", {});
Priority: -- → P3
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js]
Do we need to replace all |log| instances in Parser.jsm file or this |log| function is used somewhere else as well?
Also it seems that |log| function has more logic than |reportException| wonder what is the strategy to deal with that.

I'm currently poking around https://bugzilla.mozilla.org/show_bug.cgi?id=957634
but I can start looking into this bug as well.
Hi!

|log| is only used inside Parser.jsm

Everything that |log| does is done by the combo of |reportException| and |safeErrorString|, see: https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/DevToolsUtils.js#16
Assignee: nobody → alexey.novak.mail
Thanks!
I will take a look into this and will keep you updated on my progress.
Attached patch 956804.patch (obsolete) — — Splinter Review 
I managed to do the patch. The change was so simple that it made me worried if I missed something there.

Also was not sure how to test it. I did build fx-team and there were no errors.

Please let me know if there is anything I need to change.
Attachment #8359826 - Flags: review?(nfitzgerald)
Yup, this is a simple bug, hence the [good first bug] tag :)

Should have mentioned this before, but the Parser.jsm tests are mixed in with the debugger mochitests. This should work:

> ./mach mochitest-browser browser/devtools/debugger

https://wiki.mozilla.org/DevTools/Hacking#Browser_Mochitests
Comment on attachment 8359826 [details] [diff] [review]
956804.patch

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

Try run to verify tests are passing on all platforms: https://tbpl.mozilla.org/?tree=Try&rev=f2751d8117a2

r+ with green try. Assuming it comes back good, we'll land this!
Attachment #8359826 - Flags: review?(nfitzgerald) → review+
Oh wow. My computer is chocking running that mochitest-browser command...

The second link you sent - I'm not sure how to use it. It would be great if you could explain that tbpl link since I saw it couple of time but still not exactly sure what is it for and how to use it.
Yeah, you have to just let mochitests run, you can't alt-tab or anything. It takes about 5 mins for me to run, maybe shorter maybe longer for you.

The latest link I posted is just a try run, similar to Travis CI. It will give us test results on all of the platforms Firefox supports. You don't need to do anything with it but wait.

Learn more about Try here: https://wiki.mozilla.org/ReleaseEngineering/TryServer
since the mochitest run literally make my computer unusable for > 15 min where I just killed it and decided to do later.

So finally, I found some time and decided to pull latest default branch of fx-team and just run tests on their own without my changes and maybe after I will do the same but with my changes.

What I found that whenever I try to run the command you specified
>./mach mochitest-browser browser/devtools/debugger

My tests no longer work. I wonder if it is something my computer related or changes pushed to the default. I tried so many things but cannot make mochitests to run again... any help here would be more than welcome.


I see the same output in stdout

oz-macair:fx-team oz$ ./mach mochitest-browser browser/devtools/debuggerFrom _tests: Kept 12198 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:10.67 MochitestServer : launching [u'/Users/oz/Documents/mozilla/fx-team/obj-ff-dbg/dist/bin/xpcshell', '-g', u'/Users/oz/Documents/mozilla/fx-team/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS', '-v', '170', '-f', '/Users/oz/Documents/mozilla/fx-team/obj-ff-dbg/_tests/testing/mochitest/httpd.js', '-e', "const _PROFILE_PATH = '/var/folders/6t/kywj_y_172nc1g991bfgmtgm0000gn/T/tmphvJXX4'; const _SERVER_PORT = '8888'; const _SERVER_ADDR = '127.0.0.1'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;", '-f', './server.js']
 0:10.67 runtests.py | Server pid: 367
 0:12.68 runtests.py | Websocket server pid: 368
 0:12.68 runtests.py | Running tests: start.

pk12util: PKCS12 IMPORT SUCCESSFUL
 0:13.13 INFO | runtests.py | SSL tunnel pid: 377
 0:13.14 INFO | runtests.py | Application pid: 378
 0:13.94 ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /var/folders/6t/kywj_y_172nc1g991bfgmtgm0000gn/T/tmphvJXX4/runtests_leaks.log
 0:15.94 *** ERROR addons.xpi: Unable to read add-on manifest from /var/folders/6t/kywj_y_172nc1g991bfgmtgm0000gn/T/tmphvJXX4/extensions/staged/workerbootstrap-test@mozilla.org: [Exception... "Not enough arguments [nsIBlocklistService.getAddonBlocklistState]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: AddonInternal.prototype.blocklistState :: line 6213"  data: no]
 0:15.99 *** ERROR addons.xpi: Unable to read add-on manifest from /var/folders/6t/kywj_y_172nc1g991bfgmtgm0000gn/T/tmphvJXX4/extensions/staged/worker-test@mozilla.org: [Exception... "Not enough arguments [nsIBlocklistService.getAddonBlocklistState]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: AddonInternal.prototype.blocklistState :: line 6213"  data: no]
 0:16.02 *** ERROR addons.xpi: Unable to read add-on manifest from /var/folders/6t/kywj_y_172nc1g991bfgmtgm0000gn/T/tmphvJXX4/extensions/staged/special-powers@mozilla.org: [Exception... "Not enough arguments [nsIBlocklistService.getAddonBlocklistState]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: AddonInternal.prototype.blocklistState :: line 6213"  data: no]
 0:16.06 *** ERROR addons.xpi: Unable to read add-on manifest from /var/folders/6t/kywj_y_172nc1g991bfgmtgm0000gn/T/tmphvJXX4/extensions/staged/mochikit@mozilla.org: [Exception... "Not enough arguments [nsIBlocklistService.getAddonBlocklistState]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: AddonInternal.prototype.blocklistState :: line 6213"  data: no]
 0:16.10 *** ERROR addons.xpi: Unable to read add-on manifest from /var/folders/6t/kywj_y_172nc1g991bfgmtgm0000gn/T/tmphvJXX4/extensions/staged/indexedDB-test@mozilla.org: [Exception... "Not enough arguments [nsIBlocklistService.getAddonBlocklistState]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: AddonInternal.prototype.blocklistState :: line 6213"  data: no]
 0:16.93 ++DOCSHELL 0x11295f000 == 1 [pid = 378] [id = 1]
 0:16.93 ++DOMWINDOW == 1 (0x1129bc4b8) [pid = 378] [serial = 1] [outer = 0x0]
 0:16.94 ++DOMWINDOW == 2 (0x1131544b8) [pid = 378] [serial = 2] [outer = 0x1129bc4b8]
 0:17.22 pldhash: for the table at address 0x11325f860, the given entrySize of 168 definitely favors chaining over double hashing.
 0:17.67 [378] WARNING: No disk space watcher component available!: file /Users/oz/Documents/mozilla/fx-team/dom/indexedDB/IndexedDatabaseManager.cpp, line 224
 0:19.20 ++DOCSHELL 0x115cb7400 == 2 [pid = 378] [id = 2]
 0:19.20 ++DOMWINDOW == 3 (0x115cb80b8) [pid = 378] [serial = 3] [outer = 0x0]
 0:19.20 ++DOMWINDOW == 4 (0x1120c6cb8) [pid = 378] [serial = 4] [outer = 0x115cb80b8]
 0:19.58 -*- Webapps.jsm : Saving /var/folders/6t/kywj_y_172nc1g991bfgmtgm0000gn/T/tmphvJXX4/webapps/webapps.json
 0:19.66 -*- Webapps.jsm : Saving /var/folders/6t/kywj_y_172nc1g991bfgmtgm0000gn/T/tmphvJXX4/webapps/webapps.json
 0:20.07 [378] WARNING: Loaded script chrome://global/content/printUtils.js twice (bug 392650): file /Users/oz/Documents/mozilla/fx-team/content/xul/document/src/nsXULPrototypeCache.cpp, line 204
 0:20.42 [378] WARNING: Loaded script chrome://global/content/viewZoomOverlay.js twice (bug 392650): file /Users/oz/Documents/mozilla/fx-team/content/xul/document/src/nsXULPrototypeCache.cpp, line 204
 0:20.63 [378] WARNING: Loaded script chrome://browser/content/places/browserPlacesViews.js twice (bug 392650): file /Users/oz/Documents/mozilla/fx-team/content/xul/document/src/nsXULPrototypeCache.cpp, line 204
 0:21.75 [378] WARNING: Loaded script chrome://browser/content/browser.js twice (bug 392650): file /Users/oz/Documents/mozilla/fx-team/content/xul/document/src/nsXULPrototypeCache.cpp, line 204
 0:21.85 [378] WARNING: Loaded script chrome://browser/content/downloads/downloads.js twice (bug 392650): file /Users/oz/Documents/mozilla/fx-team/content/xul/document/src/nsXULPrototypeCache.cpp, line 204
 0:21.90 [378] WARNING: Loaded script chrome://browser/content/downloads/indicator.js twice (bug 392650): file /Users/oz/Documents/mozilla/fx-team/content/xul/document/src/nsXULPrototypeCache.cpp, line 204
 0:21.94 [378] WARNING: Loaded script chrome://browser/content/customizableui/panelUI.js twice (bug 392650): file /Users/oz/Documents/mozilla/fx-team/content/xul/document/src/nsXULPrototypeCache.cpp, line 204
 0:21.99 [378] WARNING: Loaded script chrome://global/content/inlineSpellCheckUI.js twice (bug 392650): file /Users/oz/Documents/mozilla/fx-team/content/xul/document/src/nsXULPrototypeCache.cpp, line 204
 0:22.02 [378] WARNING: Loaded script chrome://global/content/viewSourceUtils.js twice (bug 392650): file /Users/oz/Documents/mozilla/fx-team/content/xul/document/src/nsXULPrototypeCache.cpp, line 204
 0:22.26 ++DOCSHELL 0x11725e400 == 3 [pid = 378] [id = 3]
 0:22.26 ++DOMWINDOW == 5 (0x117448cb8) [pid = 378] [serial = 5] [outer = 0x0]
 0:22.26 ++DOCSHELL 0x1178bb400 == 4 [pid = 378] [id = 4]
 0:22.26 ++DOMWINDOW == 6 (0x1178bbcb8) [pid = 378] [serial = 6] [outer = 0x0]
 0:23.27 [378] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /Users/oz/Documents/mozilla/fx-team/content/base/src/nsFrameLoader.cpp, line 395
 0:23.76 ++DOCSHELL 0x11c2dac00 == 5 [pid = 378] [id = 5]
 0:23.76 ++DOMWINDOW == 7 (0x11c2db4b8) [pid = 378] [serial = 7] [outer = 0x0]
 0:23.78 [378] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /Users/oz/Documents/mozilla/fx-team/content/base/src/nsFrameLoader.cpp, line 395
 0:24.00 ++DOMWINDOW == 8 (0x11c336cb8) [pid = 378] [serial = 8] [outer = 0x11c2db4b8]
 0:24.31 [378] WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && subjPrincipal) failed: file /Users/oz/Documents/mozilla/fx-team/docshell/base/nsDocShell.cpp, line 8558
 0:24.37 [378] WARNING: NS_ENSURE_TRUE(mTextInputHandler) failed: file /Users/oz/Documents/mozilla/fx-team/widget/cocoa/nsChildView.mm, line 5294
 0:24.37 [378] WARNING: Failed to open external DTD: publicId "-//W3C//DTD SVG 1.1//EN" systemId "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd" base "file:///Users/oz/Documents/mozilla/fx-team/obj-ff-dbg/browser/themes/osx/tab-selected-start.svg" URL "resource://gre/res/dtd/svg11.dtd": file /Users/oz/Documents/mozilla/fx-team/parser/htmlparser/src/nsExpatDriver.cpp, line 696
 0:24.38 [378] WARNING: Failed to open external DTD: publicId "-//W3C//DTD SVG 1.1//EN" systemId "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd" base "file:///Users/oz/Documents/mozilla/fx-team/obj-ff-dbg/browser/themes/osx/tab-selected-end.svg" URL "resource://gre/res/dtd/svg11.dtd": file /Users/oz/Documents/mozilla/fx-team/parser/htmlparser/src/nsExpatDriver.cpp, line 696
 0:24.42 [378] WARNING: NS_ENSURE_TRUE(globalObject && globalObject->GetGlobalJSObject()) failed: file /Users/oz/Documents/mozilla/fx-team/content/html/document/src/nsHTMLContentSink.cpp, line 717
 0:24.42 [378] WARNING: Subdocument container has no frame: file /Users/oz/Documents/mozilla/fx-team/layout/base/nsDocumentViewer.cpp, line 2405
 0:24.42 ++DOMWINDOW == 9 (0x11c6b4cb8) [pid = 378] [serial = 9] [outer = 0x117448cb8]
 0:24.48 [378] WARNING: NS_ENSURE_TRUE(globalObject && globalObject->GetGlobalJSObject()) failed: file /Users/oz/Documents/mozilla/fx-team/content/html/document/src/nsHTMLContentSink.cpp, line 717
 0:24.48 [378] WARNING: Subdocument container has no frame: file /Users/oz/Documents/mozilla/fx-team/layout/base/nsDocumentViewer.cpp, line 2405
 0:24.48 ++DOMWINDOW == 10 (0x11c6b58b8) [pid = 378] [serial = 10] [outer = 0x1178bbcb8]
 0:24.54 ++DOMWINDOW == 11 (0x11c6b74b8) [pid = 378] [serial = 11] [outer = 0x11c2db4b8]
 0:24.72 OpenGL version detected: 210
 0:24.72 OpenGL vendor: Intel Inc.
 0:24.72 OpenGL renderer: Intel HD Graphics 3000 OpenGL Engine
 0:24.73 OpenGL version detected: 210
 0:24.73 OpenGL vendor: Intel Inc.
 0:24.73 OpenGL renderer: Intel HD Graphics 3000 OpenGL Engine
 0:26.43 [378] WARNING: the property saveStyleSheet.title already exists
 0:26.43 : file /Users/oz/Documents/mozilla/fx-team/xpcom/ds/nsPersistentProperties.cpp, line 536
 0:26.43 [378] WARNING: the property saveStyleSheet.title already exists
 0:26.43 : file /Users/oz/Documents/mozilla/fx-team/xpcom/ds/nsPersistentProperties.cpp, line 536
 0:26.61 [378] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /Users/oz/Documents/mozilla/fx-team/content/events/src/nsContentEventHandler.cpp, line 96
 0:26.61 [378] WARNING: NS_ENSURE_TRUE(mTextInputHandler) failed: file /Users/oz/Documents/mozilla/fx-team/widget/cocoa/nsChildView.mm, line 5294
 0:26.88 [378] WARNING: NS_ENSURE_TRUE(mMutable) failed: file /Users/oz/Documents/mozilla/fx-team/netwerk/base/src/nsSimpleURI.cpp, line 265
 0:26.93 ++DOMWINDOW == 12 (0x1159e20b8) [pid = 378] [serial = 12] [outer = 0x11c2db4b8]
 0:27.23 [378] WARNING: NS_ENSURE_TRUE(mMutable) failed: file /Users/oz/Documents/mozilla/fx-team/netwerk/base/src/nsSimpleURI.cpp, line 265
 0:31.95 ++DOCSHELL 0x112653000 == 6 [pid = 378] [id = 6]
 0:31.95 ++DOMWINDOW == 13 (0x1126570b8) [pid = 378] [serial = 13] [outer = 0x0]
 0:32.06 [378] WARNING: NS_ENSURE_TRUE(globalObject && globalObject->GetGlobalJSObject()) failed: file /Users/oz/Documents/mozilla/fx-team/content/html/document/src/nsHTMLContentSink.cpp, line 717
 0:32.06 ++DOMWINDOW == 14 (0x114cf84b8) [pid = 378] [serial = 14] [outer = 0x1126570b8]
 0:32.17 [378] WARNING: NS_ENSURE_TRUE(window) failed: file /Users/oz/Documents/mozilla/fx-team/dom/base/nsLocation.cpp, line 44
 0:32.18 ++DOMWINDOW == 15 (0x1126c7cb8) [pid = 378] [serial = 15] [outer = 0x1126570b8]
 0:32.30 ++DOCSHELL 0x100421c00 == 7 [pid = 378] [id = 7]
 0:32.30 ++DOMWINDOW == 16 (0x1114cd8b8) [pid = 378] [serial = 16] [outer = 0x0]
 0:32.37 ++DOMWINDOW == 17 (0x1123f9cb8) [pid = 378] [serial = 17] [outer = 0x1114cd8b8]
 0:32.54 ++DOMWINDOW == 18 (0x115ab08b8) [pid = 378] [serial = 18] [outer = 0x1114cd8b8]
 0:32.70 --DOMWINDOW == 17 (0x11c336cb8) [pid = 378] [serial = 8] [outer = 0x11c2db4b8] [url = about:blank]
 0:47.19 --DOMWINDOW == 16 (0x1123f9cb8) [pid = 378] [serial = 17] [outer = 0x1114cd8b8] [url = about:blank]
 0:48.88 --DOMWINDOW == 15 (0x11c6b74b8) [pid = 378] [serial = 11] [outer = 0x0] [url = about:blank]
 0:48.88 --DOMWINDOW == 14 (0x114cf84b8) [pid = 378] [serial = 14] [outer = 0x0] [url = about:blank]
>I managed to do the patch. The change was so simple that it made me worried if I missed something there.

Looks like I jinxed it :(
Does DTU.reportException display `lineNumber` and `columnNumber` properties for the exceptions if they're present? They are immensely valuable with Parser.jsm, because they tell us where exactly the syntax errors are. See bug 956568.
Comment on attachment 8359826 [details] [diff] [review]
956804.patch

Victor, it adds the |stack|, but not the |lineNumber| and |columnNumber|, but we can change that.

Alexey:

Can you add those properties to the |errorString| in |safeErrorString| in the same fashion that we add the |stack| property?

Also, you don't need to use a debug build for devtools stuff, only if you are hacking on C++ sources for whatever reason. This may be why your tests are running so slowly. Make sure you are doing an incremental build after making your changes as well (see the Hacking link I posted earlier).

Finally, rather than pasting big logs into the comments, please upload them as an attachment so we can keep the comments read-able :)

Thanks!
Attachment #8359826 - Flags: review+
>Can you add those properties to the |errorString| in |safeErrorString| in the same fashion that we add the |stack| property?

Ok will do.

>Also, you don't need to use a debug build for devtools stuff, only if you are hacking on C++ sources for whatever reason. This may be why your tests are running so slowly. Make sure you are doing an incremental build after making your changes as well (see the Hacking link I posted earlier).

I never do an overall debug build unless I pulled new default branch with changes. Always do an incremental build and then ./mach run -p development to start Nightly and see if my changes make sense. But not sure how it applies to tests because no matter what I do (overall debug build or incremental one) tests run super slow or (currently) do not run at all.

> Finally, rather than pasting big logs into the comments, please upload them as an attachment so we can keep the comments read-able :)
Will know for the next time.
Attached patch 956804.2.patch (obsolete) — — Splinter Review 
Made the change. How does it look?
Personally not sure if that string check is valid here since columnNumber and lineNumber clearly integers. Was following the existing code for stack.

Let me know if it is wrong.
Attachment #8359826 - Attachment is obsolete: true
Attachment #8360544 - Flags: review?(nfitzgerald)
Attached file mochitest_error.txt — 
Here is my file with console output to keep bug comments readable.

I made my change.
I ran ./mach build toolkit/devtools browser      to do an incremental build
I ran ./mach mochitest-browser browser/devtools/debugger     to run mochitests. And got the error. Clearly I'm missing something and it WAS working for me before. I would sit and watch Nightly build to open/close tab performing various tests... but not anymore.

Any help/ideas/suggestions would be more than welcome :)
Attachment #8360545 - Flags: feedback?(nfitzgerald)
Comment on attachment 8360544 [details] [diff] [review]
956804.2.patch

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

::: toolkit/devtools/DevToolsUtils.js
@@ +30,5 @@
>  
> +      // Attempt to attach lineNumber and columnNumber to |errorString|. If it throws an error, or
> +      // isn't a string, don't use it.
> +      try {
> +        if (aError.lineNumber && aError.columnNumber) {

This will fail if |lineNumber| or |columnNumber| is zero, we should use this check instead:

  if ("lineNumber" in aError && "columnNumber" in aError) {

@@ +33,5 @@
> +      try {
> +        if (aError.lineNumber && aError.columnNumber) {
> +          let lineNumber = aError.lineNumber.toString();
> +          let columnNumber = aError.columnNumber.toString();
> +          if (typeof lineNumber === "string") {

Let's check that they are numbers, and just let string concatenation coerce them to strings.
Attachment #8360544 - Flags: review?(nfitzgerald)
Attached patch 956804.3.diff (obsolete) — — Splinter Review 
Basically we want exactly same logic which was in the removed |log| function.

My bad. I read your comment:

>Can you add those properties to the |errorString| in |safeErrorString| in the same fashion that we add the |stack| property?

And thought that I need to apply the same logic to |lineNumber| and |columnNumber| as we have for |stack|.

Let me know if there is anything I need to improve or change in the patch.
Attachment #8360544 - Attachment is obsolete: true
Attachment #8360568 - Flags: review?(nfitzgerald)
(In reply to Alexey Novak from comment #17)
> Created attachment 8360545 [details]
> mochitest_error.txt
> 
> Here is my file with console output to keep bug comments readable.
> 
> I made my change.
> I ran ./mach build toolkit/devtools browser      to do an incremental build
> I ran ./mach mochitest-browser browser/devtools/debugger     to run
> mochitests. And got the error. Clearly I'm missing something and it WAS
> working for me before. I would sit and watch Nightly build to open/close tab
> performing various tests... but not anymore.
> 
> Any help/ideas/suggestions would be more than welcome :)

If you pop off your patch, do an incremental build, and run the tests, do they work?

If so, you know the problem is related to your changes.

If not, then I would restart:

1. Change your .mozconfig so you aren't running a debug build

2. ./mach clobber

3. ./mach build

4. Run the tests.
(In reply to Alexey Novak from comment #19)
> Created attachment 8360568 [details] [diff] [review]
> 956804.3.diff
> 
> Basically we want exactly same logic which was in the removed |log| function.
> 
> My bad. I read your comment:
> 
> >Can you add those properties to the |errorString| in |safeErrorString| in the same fashion that we add the |stack| property?
> 
> And thought that I need to apply the same logic to |lineNumber| and
> |columnNumber| as we have for |stack|.
> 
> Let me know if there is anything I need to improve or change in the patch.

Yeah I didn't mean *exactly* the same, sorry.
Nah. was my fault, should not take everything literally. I was writing the code the same way as stack and it was screaming at me - "it does not make sense".

I will try your suggestions ^ and will keep you updated on my progress.
Thanks for your time, feedback and suggestions. Much appreciate it!
Attachment #8360545 - Flags: feedback?(nfitzgerald)
Comment on attachment 8360568 [details] [diff] [review]
956804.3.diff

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

LGTM, here is another try push: https://tbpl.mozilla.org/?tree=Try&rev=7de1b89a9aca

Were you able to get the tests passing?
Attachment #8360568 - Flags: review?(nfitzgerald) → review+
looks like Try gives an error in tests.

Unfortunately, I still cannot run tests. Same error as my attachment before.
At the same time I noticed that I messed up my fx-team build. It always spits an error of type:

ld: warning: could not create compact unwind for _ffi_call_unix64: does not use RBP or RSP based frame
Undefined symbols for architecture x86_64:
(should search more about this problem)

But if I run build command again it would run for 4 minutes instead of 46 minutes as the first fail build attempt 
./mach run would work perfectly fine and usable. Can do changes in the code and run stuff... although something tells me that that error ^ would be the cause of tests not to run and fail.

Will poke around build and tests again tomorrow.

Maybe if you could take a look at my .mozconfig file and see if anything is missing or funky there. (I run my system on OSX 10.7.5 with XCode 4.6.2)
Attached file mozconfig.txt — 
my mozconfig file
The errors you were getting about "[nsIBlocklistService.getAddonBlocklistState]" when running mochitests were due to some bad code that was checked in at the moment you had pulled down the code.  If you update your local repo and rebuild, those should go away.

> ld: warning: could not create compact unwind for _ffi_call_unix64: does not
> use RBP or RSP based frame
> Undefined symbols for architecture x86_64

I also get this message frequently, it's harmless.

> Maybe if you could take a look at my .mozconfig file

The MOZ_MAKE_FLAGS line is no longer needed when using mach.  It will figure out the right number of jobs based on how many CPU cores you have.

Generally though, your .mozconfig seems fine.  I'll bet updating your local repo will fix it.
Thanks Ryan! It is exactly what you said. I pulled the latest version of fx-team and do not see that error anymore. Thanks a lot.

Nick, I will run the tests today in the evening and see if there are any problems.


On a side note:
Nick, I'm not sure about the statement "I also get this message frequently, it's harmless."

My first run of the build command always fails. It fails on ~44 min of my build at [js] point:

44:10.54 libjs_static.a.desc
44:10.91 libeditline.a.desc
44:11.05 js
44:17.22 Executing: /usr/local/bin/ccache clang++-mp-3.3 -o js -Qunused-arguments -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Werror=conversion-null -Wsign-compare -Wno-invalid-offsetof -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags -fno-common -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -DNDEBUG -DTRIMMED -g -O3 -fno-stack-protector -fomit-frame-pointer -Wl,-filelist,/Users/oz/Documents/mozilla/fx-team/obj-ff-dbg/js/src/js/src/shell/tmp6YxRp0.list -lobjc -framework ExceptionHandling -Wl,-executable_path,../../../../../dist/bin -Wl,-dead_strip -L../../../../../dist/bin -L../../../../../dist/lib -L/Users/oz/Documents/mozilla/fx-team/obj-ff-dbg/dist/lib -lnspr4 -lplc4 -lplds4 ../libjs_static.a -L../../../../../dist/lib -lmozglue -lm
...
...
tones of errors below


And then immediately fails the whole thing with:

44:17.88 ld: symbol(s) not found for architecture x86_64
44:17.88 clang: error: linker command failed with exit code 1 (use -v to see invocation)
44:17.88 gmake[7]: *** [js] Error 1
44:17.88 gmake[6]: *** [js/src/shell/libs] Error 2
44:17.88 gmake[5]: *** [libs] Error 2
44:17.88 gmake[4]: *** [js/src/libs] Error 2
44:17.88 gmake[3]: *** [libs] Error 2
44:17.88 gmake[2]: *** [default] Error 2
44:17.88 gmake[1]: *** [realbuild] Error 2
44:17.88 gmake: *** [build] Error 2


It is only when I run build command for the second time -> build will take just few minutes, will be less verbose and will build everything just fine. But that error ^ cost me hours and hours trying to figure out what had failed for me until I ran build command for the second time by accident. If not that I might've still were stuck on the build command.
I've never had that message or this issue, so I don't know how to help. Maybe :jryans can help, if not try stopping by #developers on irc and asking there :-/

The try push looks like errors in the test for safeErrorString. You can run that test individually with:

  ./mach xpcshell-test toolkit/devtools/tests/unit/test_safeErrorString.js

Probably just needs to be updated to handle the line and column info.
Attached patch 956804.4.patch (obsolete) — — Splinter Review 
Made the change and modified the test.

The failure was caused by the following line:
>if ("lineNumber" in aError && "columnNumber" in aError) {

That check was working perfectly before in the |log| function because |log| would always expect an exception object aEx passed in. It is a different story with |reportException| since it can expect anything (e.g. tests pass different variety of arguments and not necessarily an Exception object)

So I did some research and looked for the best way to check for property existence. Obviously |aError.lineNumber| would not work since lineNumber can simply be 0 which immediately False our IF statement.
So I found another approach to the problem |typeof obj.propery != 'undefined'| and went one step further by checking if lineNumber and columnNumber are numbers.
So the check would look like:
>if (typeof aError.lineNumber === 'number' && typeof aError.columnNumber === 'number') {

Let me know if it is a decent solution or you have a better one on your mind. 



On the build note. Thanks for suggestions. I will definitely stop by in IRC if this build process would bother me. So far it seems that things work so far... it becomes an annoyance when I need to rebuild the whole fx-team project.
Attachment #8360568 - Attachment is obsolete: true
Attachment #8361403 - Flags: review?(nfitzgerald)
Comment on attachment 8361403 [details] [diff] [review]
956804.4.patch

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

So tests and everything are up and running for you now?

This looks pretty good to me, once you supply these final changes I will do one last try run to make sure it is dandy and then land the code.

Thanks for sticking with it :)

::: toolkit/devtools/DevToolsUtils.js
@@ +27,5 @@
>            }
>          }
>        } catch (ee) { }
>  
> +      if (typeof aError.lineNumber === 'number' && typeof aError.columnNumber === 'number') {

Nits: Use == when you know you are comparing two strings, and double quotes instead of single quotes.
Attachment #8361403 - Flags: review?(nfitzgerald)
Attached patch 956804.5.patch (obsolete) — — Splinter Review 
Made changes.
Attachment #8361403 - Attachment is obsolete: true
Attachment #8362013 - Flags: review?(nfitzgerald)
Attached patch 956804.6.patch — — Splinter Review 
Actually I saw some more examples in the file where === is used for string comparison. Decided to fix them as well since I'm working on this file anyway.
Attachment #8362013 - Attachment is obsolete: true
Attachment #8362013 - Flags: review?(nfitzgerald)
Attachment #8362015 - Flags: review?(nfitzgerald)
Comment on attachment 8362015 [details] [diff] [review]
956804.6.patch

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

Thanks!

Try push: https://tbpl.mozilla.org/?tree=Try&rev=dad7e1606daa

Assuming all comes back well, I'll land this for you :)
Attachment #8362015 - Flags: review?(nfitzgerald) → review+
Any update on this issue?
Test returned green. Local test command you suggested ^ also pass OK.
In the try push there are some orange in (bc) is it what I'm thinking? :)
Flags: needinfo?(nfitzgerald)
I've been on a small vacation, but I'm back now.

Looks like the try push is failing on mochitests, but seems unrelated. Could just be caught up with some bad push that landed and has since been backed out.

Will do another try push and see if it is better now.
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(nfitzgerald)
Argh looks like the try server infrastructure has been having issues and was recently reset. Notice how all the B2G builds are broken in that push. Sigh...

I'm not going to do another push because that looks pretty good, and I don't think anything is broken since both of us have passing tests locally and all the stuff that didn't get caught by try being broken was good.
Flags: needinfo?(nfitzgerald)
https://hg.mozilla.org/integration/fx-team/rev/67d36c90a7ea
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js] → [fixed-in-fx-team]
Thanks for helping me with this bug and guiding from the beginning till the end.
I also believe that it should not break anything much since it is a very minor change.

Nick, do you have any good suggestion for the next bug or just go ahead and pick another one from the mentored list?
Sorry that there seemed to be a lot more little hang ups with this bug than usual, what with getting a broken revision checked out. I should have asked you to pull the latest code down earlier. My bad!

Anyways, I think finding a non-good-first-bug, but still mentored bug would be good. If any other bug catches your eye, feel free to ask me or someone else in IRC if they think it would be a good fit. Here are a few that might be good:

https://bugzilla.mozilla.org/show_bug.cgi?id=878308
https://bugzilla.mozilla.org/show_bug.cgi?id=918416
https://bugzilla.mozilla.org/show_bug.cgi?id=943341
https://hg.mozilla.org/mozilla-central/rev/67d36c90a7ea
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.