Closed Bug 809561 Opened 12 years ago Closed 10 years ago

Integrate xpcshell test harness with chrome remote debugging

Categories

(Testing :: XPCShell Harness, defect)

Product:
Testing
Component:
XPCShell Harness
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla37

People

(Reporter: gps, Assigned: markh)

References

Details

Attachments

(1 file, 7 obsolete files)

0001-Bug-809561-Integrate-xpcshell-test-harness-with-chro.patch
28.62 KB, patch
Details | Diff | Splinter Review 
I was talking with dcamp about getting the new chrome debugger to work with xpcshell tests. He said it's possible.

Essentially the xpcshell process needs to start up the debugger server. Then, we can launch Firefox and point the chrome debugger at the xpcshell process.

Division of work is roughly:

1) Ability for xpcshell process to start a debugger server on a port (likely implemented as part of the xpcshell test harness's head.js).
2) Hook up #1 with runxpcshelltests.py
3) Update frontend tools to pass necessary options to #2.

I'd happily work on this because I could make use of it. But, I'm busy with other things right now. If anyone else wants to jump on board, please do.
This shouldn't be too much of a pain to do, whoever works on this can take a look at the devtools xpcshell tests. The only thing I'd like to ask is that whatever is done is not done Firefox specific. The easy way out is to call DebuggerServer.addBrowserActors() after the init, this will load all Firefox specific actors. What we would need is some way to specify what kind of actor implementation is used.

As only the chrome debugger and maybe the profiler is really useful for xpcshell tests, maybe it would make sense to create an xpcshell specific root actor that only loads the chrome debugger and provides an empty tablist implementation (or better yet, fix devtools to not require a tabList implementation).
The context that was never posted in this bug is that dcamp and I tried to do this, but it isn't as simple as one would think. We either need to resolve bug 716647 or work around it. Someone familiar with both the C++ and JS side of things could probably do it with moderate effort. It is beyond my knowledge, however.

My previous attempt is at https://hg.mozilla.org/users/gszorc_mozilla.com/gecko-patches/raw-file/default/xpcshell-debugger
Is this work finished or not? I need to make remote debugger work in xpcshell, can I find anybody who can guide me on that, I'll publish results if needed.

I need this functionality, I am working on crossplatform framework implementation nativly rendering JavaScript applications and I do need this possibility to debug applications working inside SpiderMonkey.
(In reply to Gregory Szorc [:gps] from comment #2)
> The context that was never posted in this bug is that dcamp and I tried to
> do this, but it isn't as simple as one would think. We either need to
> resolve bug 716647 or work around it.

The above bug appears to have landed.  What are the next steps?
I'd love this, but sadly the WIP from comment 2 is quite stale and I'm finding the "actors" setup quite intractable...
This is a WIP, originally based on the one from gps, but modernized with help from past and ejpbruel.  It seems so close to working, but still no cigar.

The client successfully connects and then lists the "tabs" which can be attached to - in this case the one tab with a title "Testing, testing...", as setup by this patch.  I then see the TabActor attached and a few packets bounce around, then the webconsole opens, but nothing else happens.  I *do not* see my ThreadActor attached to - indeed, I see no evidence of any packets targetting that actor at all.

It's likely I'm mis-interpreting the advice of past here, but thought I'd put it up for comment and additional help.

To test the patch, just apply it and execute "./mach xpcshell-test path/to/any/single/xpcshell_test.js".  Then wait to see the output "Waiting for our thread actor to attach" and connect from a Firefox instance.

Next up I'll attach the log I see.
Flags: needinfo?(past)
Flags: needinfo?(ejpbruel)
Attached file xpcshell output (obsolete) — 
This is the output from the xpcshell (ie, server) side.
(In reply to Mark Hammond [:markh] from comment #6)
> The client successfully connects and then lists the "tabs" which can be
> attached to - in this case the one tab with a title "Testing, testing...",
> as setup by this patch.  I then see the TabActor attached and a few packets
> bounce around, then the webconsole opens, but nothing else happens.  I *do
> not* see my ThreadActor attached to - indeed, I see no evidence of any
> packets targetting that actor at all.

You need to open the debugger panel, as that is what we are trying to make functional first with the ThreadActor. The WebConsoleActor needs to be taken care of next, if we want to make the console and scratchpad functional, too.
Moving this:

Services.obs.notifyObservers(null, "xpcshell-debugger-attached", null);

to onResume in script.js, right after "this._popThreadPause();" makes things almost work :-)
Flags: needinfo?(past)
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #9)
> Moving this:
> 
> Services.obs.notifyObservers(null, "xpcshell-debugger-attached", null);
> 
> to onResume in script.js, right after "this._popThreadPause();" makes things
> almost work :-)

It does - thanks! :)  And with this patch it *actually* works \o/.  No attempt at console integration yet, but this seems to give us complete debugger functionality.

This patch:

* Includes a couple of changes to script.js - it sends the observer notification (sending |this| - ie, the ThreadActor, as the |subject|) and also adds a |this.wrappedJSObject = this| in the constructor so the observer itself can use the ThreadActor (otherwise it just sees a generic nsISupports without an ability to unwrap it)

* The observer in xpcshell's head.js uses the ThreadActor to preemptively add a breakpoint on the first line of the test being run.  With this change, the debugger correctly stops in the test itself rather than in the harness.  Unfortunately I had to use the private _createAndStoreBreakpoint() method to make this work.

past: requesting feedback specifically on these changes (and, or course, anything else you like.)

* Adds a new "--js-debugger-port" cmdline option to mach and the test harness itself.  This command-line option is what triggers the attempt to attach to the debugger - ie, "./mach xpcshell-test --js-debugger-port=6000 path/to/test.js" is how this is invoked to see the behaviour.  After doing this I felt a better cmdline arg might be, eg, "--devtools-port" - opinions?

gps: requesting feedback from you on this (and again, anything else you like)

Everyone else: feel free to give it a go - works as advertised for me.

Even though there are a few paper-cuts, I think it would be awesome if we can land this with the functionality it already has and fix these paper-cuts in a followup - IMO it is already useful enough that people can take advantage of it even without some of the obvious polish we could do.
Attachment #8520261 - Attachment is obsolete: true
Attachment #8520262 - Attachment is obsolete: true
Flags: needinfo?(ejpbruel)
Attachment #8522010 - Flags: feedback?(past)
Attachment #8522010 - Flags: feedback?(gps)
Comment on attachment 8522010 [details] [diff] [review]
0001-Bug-809561-Integrate-xpcshell-test-harness-with-chro.patch

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

Looks promising! But I am so far removed from this original PoC that I don't think I'll be of much use on subsequent reviews.

Please run the xpcshell harness changes past someone in A*Team for final review.
Attachment #8522010 - Flags: feedback?(gps) → feedback+
I noticed that alot of the boilerplate in testing/xpcshell/dbg-actors.js isn't actually necessary, so I removed that - but see also bug 1099474, which removes the tab actor entirely.  I also tweaked things so if the toolbox is closed before the debugger connects things work correctly.
Attachment #8522010 - Attachment is obsolete: true
Attachment #8522010 - Flags: feedback?(past)
Attachment #8523332 - Flags: feedback?(past)
Comment on attachment 8523332 [details] [diff] [review]
0001-Bug-809561-Integrate-xpcshell-test-harness-with-chro.patch

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

It is great that you got this to work! Using the "Connect" screen is fine for now, but ultimately we will probably want to have it run from WebIDE, as that is our main UI for remote connections going forward. That's definitely not important right now.

It seems like we don't need tab actors at all, since we are connecting to the global console actor and the chrome debugging actor. Returning an empty tab list should be enough to make things work.

::: testing/xpcshell/dbg-actors.js
@@ +82,5 @@
> +    findDebuggees: () => [],
> +    shouldAddNewGlobalAsDebuggee: () => true,
> +  });
> +
> +  this.traits = { reconfigure: true }; // ???

This shouldn't be necessary, being able to toggle JS and caching isn't useful in this case.

::: testing/xpcshell/head.js
@@ +371,5 @@
> +        // Add a breakpoint for the first line in our test files.
> +        let threadActor = subject.wrappedJSObject;
> +        for (let file of _TEST_FILE) {
> +          let location = {url: file, line: 1};
> +          threadActor._createAndStoreBreakpoint(location);

Hacky, but I like it! Just make that method public and add a comment about why.

::: testing/xpcshell/mach_commands.py
@@ +424,5 @@
>                              "stdout and stderr for interactive debuggers")
> +    @CommandArgument("--js-debugger-port", type=int, dest="jsDebuggerPort",
> +                     default=0,
> +                     help="Waits for a devtools JS debugger to connect on the "
> +                           "specified port.")

Wouldn't it be better to automatically pickup a default port number instead of requiring that the user specifies one? That would more closely mirror what the other test suites are doing.

::: toolkit/devtools/server/actors/script.js
@@ +526,5 @@
>    this.onNewSource = this.onNewSource.bind(this);
>    this.uncaughtExceptionHook = this.uncaughtExceptionHook.bind(this);
>    this.onDebuggerStatement = this.onDebuggerStatement.bind(this);
>    this.onNewScript = this.onNewScript.bind(this);
> +  this.wrappedJSObject = this; // so |this| can be sent via the observer svc.

Please add a comment that this is used by the xpcshell test harness.

@@ +1077,5 @@
>        }
>  
>        let packet = this._resumed();
>        this._popThreadPause();
> +      Services.obs.notifyObservers(this, "devtools-thread-resumed", null);

Ditto.

::: toolkit/devtools/server/actors/utils/map-uri-to-addon-id.js
@@ +14,5 @@
>    get: (function () {
>      let cached;
> +    return () => {
> +      if (cached === undefined) {
> +        // the addonManager might not exist in this environment (eg, xpcshell)

At first I thought the comment was about the if clause, could you perhaps add something like "Use a try/catch because..."?
Attachment #8523332 - Flags: feedback?(past) → feedback+
Where any changes to the Connect screen needed for this to work?
(In reply to J. Ryan Stinnett [:jryans] from comment #14)
> Where any changes to the Connect screen needed for this to work?

Ah, I guess they are in bug 1099474.
Thanks for the feedback.  There's one item I'd like clarification on.

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #13)

> ::: testing/xpcshell/mach_commands.py
> @@ +424,5 @@
> >                              "stdout and stderr for interactive debuggers")
> > +    @CommandArgument("--js-debugger-port", type=int, dest="jsDebuggerPort",
> > +                     default=0,
> > +                     help="Waits for a devtools JS debugger to connect on the "
> > +                           "specified port.")
> 
> Wouldn't it be better to automatically pickup a default port number instead
> of requiring that the user specifies one? That would more closely mirror
> what the other test suites are doing.

IIUC, the other test suites are able to arrange for the toolbox to open automatically in-process, so picking a random port there makes sense - the user never needs to copy/paste that port number.

However, in this environment we require the user to switch to another process and manually establish a connection by typing the relevant port number into that process.  It seems that picking a random port just adds friction to this process (the user would have to sift through the test output to find the line reporting the port number, then copy that number into the connect dialog or webide).  Am I missing something?

And FTR:
(In reply to J. Ryan Stinnett [:jryans] from comment #14)
> Where any changes to the Connect screen needed for this to work?

No changes to the connect screen are *necessary* to make this work - the changes in that other bug are more about "sugar"
Flags: needinfo?(past)
I guess I'm mostly asking for the mach command line switch to be similar (or better yet identical) to the mochitest one. This could be handled by always using the default port number 6000 and maybe pick a random one if the default port is in use. The user will certainly look at the log output if the connection times out.

Accepting a port number in the command line is fine, but making it mandatory is not that great. Does that make sense?
Flags: needinfo?(past)
(In reply to Gregory Szorc [:gps] from comment #11)
> Looks promising! But I am so far removed from this original PoC that I don't
> think I'll be of much use on subsequent reviews.
> 
> Please run the xpcshell harness changes past someone in A*Team for final
> review.

Thanks.  Chris, are you able to have a look at this?  It touches mach_commands.py, runxpcshelltest.py and xpcshell-test's head.js to integrate with the debugger.  The patch also touches some devtools specific stuff which Past is helping me with and reviewing.


(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #13)
> It is great that you got this to work! Using the "Connect" screen is fine
> for now, but ultimately we will probably want to have it run from WebIDE, as
> that is our main UI for remote connections going forward. That's definitely
> not important right now.

Sadly that fails due to something else expecting a "window" - but yeah, let's get that working in a followup.

> It seems like we don't need tab actors at all ...
> This shouldn't be necessary ...
> Hacky, but I like it! Just make that method public and add a comment ...

All done!

> Wouldn't it be better to automatically pickup a default port number instead
> of requiring that the user specifies one? That would more closely mirror
> what the other test suites are doing.

What's I've done here is to add a --jsdebugger "boolean" option to match the mochitest options.  There's also a --jsdebugger-port which defaults to 6000, and we can do something smarter about using a default port in a followup if that's OK with you.

> Please add a comment that this is used by the xpcshell test harness. ...
> Ditto. ...
> At first I thought the comment was about the if clause, could you ...

All done too - so hopefully this is ready for review!
Assignee: nobody → mhammond
Attachment #8523332 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8527283 - Flags: review?(past)
Attachment #8527283 - Flags: review?(cmanchester)
Now with more working when --jsdebugger is *not* passed :)  (In that case we didn't pass any value for _JSDEBUGGER_PORT to the xpcshell process, and seeing that's using strict mode, we failed as we checked if it had a value)
Attachment #8527283 - Attachment is obsolete: true
Attachment #8527283 - Flags: review?(past)
Attachment #8527283 - Flags: review?(cmanchester)
Attachment #8527449 - Flags: review?(past)
Attachment #8527449 - Flags: review?(cmanchester)
Comment on attachment 8527449 [details] [diff] [review]
0001-Bug-809561-Integrate-xpcshell-test-harness-with-chro.patch

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

Really excellent to see this happening! Feel free to re-request review or proceed with nits fixed.

::: testing/xpcshell/head.js
@@ +335,5 @@
>  
>    protocolHandler.setSubstitution("testing-common", modulesURI);
>  }
>  
> +function initDebugging(port) {

Globals in head.js get exposed to in all test files, so I guess prefix this with an underscore.

@@ +377,5 @@
> +        break;
> +      case "xpcshell-test-devtools-shutdown":
> +        // the debugger has shutdown before we got a resume event - nothing
> +        // special to do here.
> +        break;

I'm taking this function to be part of the other review, but this switch is confusing me: are we really initialized after getting the shutdown notification? Does this switch need a default?

@@ +394,5 @@
> +  dump("Waiting for the debugger to connect on port " + port + "\n")
> +  dump("\nTo connect the debugger, open a Firefox instance, select 'Develop'\n");
> +  dump("from the hamburger menu, select 'Connect' and specify the port as ");
> +  dump(port + "\n");
> +  dump("*******************************************************************\n");

_testLogger.info or do_print will give nicer output.

::: testing/xpcshell/mach_commands.py
@@ +64,5 @@
>  
>      def run_test(self, test_paths, interactive=False,
>                   keep_going=False, sequential=False, shuffle=False,
>                   debugger=None, debuggerArgs=None, debuggerInteractive=None,
> +                 jsDebugger=False, jsDebuggerPort=0,

I might default jsDebuggerPort to None here. I got a little confused between this default and '6000', below.

::: testing/xpcshell/moz.build
@@ +11,5 @@
>  ]
> +
> +TESTING_JS_MODULES += [
> +    'dbg-actors.js',
> +]

This looks sensible enough, but I'd have a build peer check this out if you aren't sure.

::: testing/xpcshell/runxpcshelltests.py
@@ +606,5 @@
>              args.insert(0, '-d')
>  
> +        port = 0 if self.jsDebuggerInfo is None else self.jsDebuggerInfo.port
> +        cmdJSD = ['-e', 'const _JSDEBUGGER_PORT = %d;' % port]
> +

This should go in buildCmdHead.

@@ +1104,5 @@
>          self.mozInfo = mozInfo
>          self.testingModulesDir = testingModulesDir
>          self.pluginsPath = pluginsPath
>          self.sequential = sequential
> +        self.jsDebugger = jsDebugger

I think this line can go away.

@@ +1206,5 @@
>  
> +        if self.jsDebuggerInfo:
> +            # The js debugger magic needs more work to do the right thing
> +            # if debugging multiple files.
> +            # XXX - markh is confused why self.singleFile is false here?

singleFile looks like dead code at this point, I really couldn't say why though.
Attachment #8527449 - Flags: review?(cmanchester) → review+
Comment on attachment 8527449 [details] [diff] [review]
0001-Bug-809561-Integrate-xpcshell-test-harness-with-chro.patch

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

The code changes look good, but using the patch results in two problems that need fixing. The precise command I used was "mach xpcshell-test --jsdebugger toolkit/devtools/server/tests/unit/test_dbgclient_debuggerstatement.js":

- toolkit/devtools/server/testsunit/head_dbg.js uses const to declare DebuggerServer, which causes an error since you already declare it in head.js
- the test harness gets into an infinite loop when shutting down:

"Handler function DebuggerTransport.prototype.onOutputStreamReady threw an exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIWindowMediator.getEnumerator]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webbrowser.js :: allAppShellDOMWindows :: line 58"  data: no]"
"Stack: allAppShellDOMWindows@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:58:10"
"sendShutdownEvent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:78:2"
"onShutdown@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://testing-common/dbg-actors.js:31:6"
"RootActor.prototype.disconnect@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/root.js:245:6"
"AP_cleanup@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/common.js:284:6"
"DSC_onClosed@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1583:4"
"DebuggerTransport.prototype.close@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:203:6"
"DebuggerTransport.prototype.onOutputStreamReady<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:269:6"
"makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:13"
"Line: 58, column: 0"

::: toolkit/devtools/server/actors/script.js
@@ +547,5 @@
>    this.onNewSource = this.onNewSource.bind(this);
>    this.uncaughtExceptionHook = this.uncaughtExceptionHook.bind(this);
>    this.onDebuggerStatement = this.onDebuggerStatement.bind(this);
>    this.onNewScript = this.onNewScript.bind(this);
> +  // so |this| can be sent via the observer svc for the xpcshell harness.

Nit: "Set a wrappedJSObject property so |this| can..."
Attachment #8527449 - Flags: review?(past) → feedback+
Thanks for the reviews!

(In reply to Chris Manchester [:chmanchester] from comment #20)

I addressed all your feedback apart from the items below.  Another look can't hurt, so re-requesting review

> @@ +377,5 @@
> > +        break;
> > +      case "xpcshell-test-devtools-shutdown":
> > +        // the debugger has shutdown before we got a resume event - nothing
> > +        // special to do here.
> > +        break;
> 
> I'm taking this function to be part of the other review, but this switch is
> confusing me: are we really initialized after getting the shutdown
> notification? Does this switch need a default?

The switch doesn't really need a default as the topics we listen for are in a const above.  Re the "shutdown" - that really just means the *debugger* has shut-down.  This is needed if the user connects with the debugger, but then closes the devtools toolbox before switching to the debugger tab.  So in effect this means "the debugger *almost* connected but shutdown before resuming our thread"

> This looks sensible enough, but I'd have a build peer check this out if you
> aren't sure.

I think that's fine (and worst case I just need to beg forgiveness ;)


(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #21)
> Comment on attachment 8527449 [details] [diff] [review]

> - toolkit/devtools/server/testsunit/head_dbg.js uses const to declare
> DebuggerServer, which causes an error since you already declare it in head.js

Right - Cu.import() and the magic "inject into the global" behaviour :(  I fixed this by doing the Cu.import() into a local object, so DebuggerServer becomes a local.

> - the test harness gets into an infinite loop when shutting down:

Hrm - yeah, I'm surprised I didn't see that myself before, but I fixed it.

One other thing I'm seeing now but don't recall seeing before is that when using the debugger I now see the tests fail due to an unhandled promise rejection:


 0:13.57 LOG: Thread-1 ERROR  A promise chain failed to handle a rejection: Failed to open input source 'file:///' - rejection date: Wed Nov 26 2014 18:12:41 GMT+1100 (AUS Eastern Standard Time)
JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js :: fetch :: line 454
JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js :: SourceActor.prototype._getSourceText :: line 2769

The fact we are trying to load file:/// is something that should ultimately be fixed (we are also trying to load a script named '-e' due to how xpcshell works, but I think we can fix that later too).  However, even though this promise rejection *is* handled in almost every case, the one place it isn't is the "local" promise in _getSourceText:

    sourceFetched.then(({ contentType }) => {
      this._contentType = contentType;
    });

Because this doesn't have a final .then(null, err => ...) it is considered unhandled.  I fixed this in the patch too - I figured that instead of adding the trailing rejection handler I'd just have the promise returned by getSourceText() be resolved *after* this._contentType was set.

> Nit: "Set a wrappedJSObject property so |this| can..."

Fixed too.
Attachment #8527449 - Attachment is obsolete: true
Attachment #8528873 - Flags: review?(past)
Attachment #8528873 - Flags: review?(cmanchester)
Comment on attachment 8528873 [details] [diff] [review]
0001-Bug-809561-Integrate-xpcshell-test-harness-with-chro.patch

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

The changes look good, but I couldn't get it to work this time, presumably due to a bad merge that I did locally (the patch needs rebasing for fx-team tip). r=me assuming it works well for you after you rebase.
Attachment #8528873 - Flags: review?(past) → review+
Comment on attachment 8528873 [details] [diff] [review]
0001-Bug-809561-Integrate-xpcshell-test-harness-with-chro.patch

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

::: testing/xpcshell/head.js
@@ +394,5 @@
> +  do_print("*******************************************************************");
> +  do_print("Waiting for the debugger to connect on port " + port)
> +  do_print("")
> +  do_print("To connect the debugger, open a Firefox instance, select 'Develop'");
> +  do_print("Use the Developer menu, select 'Connect' and specify the port as " + port);

This could be re-worded slightly, something like "...open a Firefox instance, select 'Connect' from the Developer menu, and specify the port as...'
Attachment #8528873 - Flags: review?(cmanchester) → review+
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #23)
> r=me assuming it works well for you after you rebase.

It doesn't :(  Bug 905700 wreaked some havoc with this :(

I've nearly got it working again, but I've struck an issue I think might be fallout from bug 905700.  I pick a random test, eg:

% ./mach xpcshell-test --jsdebugger test_addon_utils.js

As soon as I connect and the debugger resumes, I get the following stack trace:

> DBG-SERVER: Got an exception: No breakpoint at = o:/src/mozilla-git/gecko-dev/obj-release/_tests/xpcshell/services/sync/tests/unit/head_appinfo.js, line = 1, column = undefined
> BreakpointStore.prototype.getBreakpoint@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:220:1
> SourceActor.prototype._getOrCreateBreakpointActor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:2948:22
> SourceActor.prototype._setBreakpoint@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:3075:36
> ThreadActor.prototype._addScript@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:2172:9
> ThreadActor.prototype.onNewScript@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:2095:5
> load_file@o:\\src\\mozilla-git\\gecko-dev\\testing\\xpcshell\\head.js:553:7
> _load_files@o:\\src\\mozilla-git\\gecko-dev\\testing\\xpcshell\\head.js:565:3
> _execute_test@o:\\src\\mozilla-git\\gecko-dev\\testing\\xpcshell\\head.js:452:3
> @-e:1:1

This exception happens as the next file is imported after attaching to the debugger - in this case it's services/sync/tests/unit/head_appinfo.js.  I believe the debugger is looking for breakpoints in this source file (and there are none; this patch does pre-set breakpoints, but for the test itself - this is just a "head.js" import) - it ends up:
* Calling SourceActor._getOrCreateBreakpointActor()
* This immediately calls BreakpointStore.getBreakpoint()
* This explicitly throws when there's no breakpoint at that location (which there isn't).

The fact something called "getOrCreate..." immediately fails when there's no existing item smells like a bug to me (and it's not clear what _getOrCreateBreakpointActor() should do in this case, else I'd have tried it ;).

:jlongster, you landed that bug - are you able to help me work out what's going wrong here or have any insights?
Attachment #8528873 - Attachment is obsolete: true
Flags: needinfo?(jlong)
(I should mention this might not be an actual problem and just be log noise - I'm not sure yet)
(In reply to Mark Hammond [:markh] from comment #25)

> It doesn't :(  Bug 905700 wreaked some havoc with this :(

It turns out this is just bug 1105493 - I was using the devtools client that didn't have the changes from bug 905700.  It all works fine with a local build.

My most recent try push had one additional problem - our call to Services.obs.notifyObservers failed as Services.obs is undefined in the test devtools/server/tests/unit/test_breakpoint-18.js.  Other globals seem similarly hobbled and I assume it is related to this code executing in a sandbox or similar when run via that test.  I just wrapped the call to Services.obs.notifyObservers with an "if (Services.obs)".

Hopefully this is the last problem - a new try is at https://tbpl.mozilla.org/?tree=Try&rev=1c9f801bff7f - I'll land it if that goes green (or unless someone objects to that Services.obs check in the meantime)
Flags: needinfo?(jlong)
(In reply to Mark Hammond [:markh] from comment #27)
> It turns out this is just bug 1105493 - I was using the devtools client that
> didn't have the changes from bug 905700.  It all works fine with a local
> build.

Ah, that's what I did too.

> My most recent try push had one additional problem - our call to
> Services.obs.notifyObservers failed as Services.obs is undefined in the test
> devtools/server/tests/unit/test_breakpoint-18.js.  Other globals seem
> similarly hobbled and I assume it is related to this code executing in a
> sandbox or similar when run via that test.  I just wrapped the call to
> Services.obs.notifyObservers with an "if (Services.obs)".

Does that mean that those tests cannot be debugged? I couldn't find a link to your previous try push, how many tests are these?
(In reply to Mark Hammond [:markh] from comment #27)
> My most recent try push had one additional problem - our call to
> Services.obs.notifyObservers failed as Services.obs is undefined in the test
> devtools/server/tests/unit/test_breakpoint-18.js.  Other globals seem
> similarly hobbled and I assume it is related to this code executing in a
> sandbox or similar when run via that test.  I just wrapped the call to
> Services.obs.notifyObservers with an "if (Services.obs)".
Can you maybe just not use Services.obs but instead Components.classes directly?
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #28)
> Does that mean that those tests cannot be debugged? I couldn't find a link
> to your previous try push, how many tests are these?

Exactly 1 - test_breakpoint-18.js - and yeah, that test isn't going to be able to be debugged.

(In reply to Philipp Kewisch [:Fallen] from comment #29)
> Can you maybe just not use Services.obs but instead Components.classes
> directly?

Cc and components are both undefined in this case too.  If you are interested, just remove the check for Services.obs in script.js and you will see that test blow up.

But I hope you all agree we can look at this in a followup, so I pushed it \o/

https://hg.mozilla.org/integration/fx-team/rev/adc66033b918
https://hg.mozilla.org/mozilla-central/rev/adc66033b918
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Sorry! Looks like my email client started flagging bugzilla emails as spam :/ Glad you got it working...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: