Closed Bug 1010750 Opened 10 years ago Closed 10 years ago

Cannot connect remote dev tools to fennec nightly

Categories

(DevTools :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox31 unaffected, firefox32- affected)

RESOLVED FIXED
Firefox 32
Tracking Status
firefox31 --- unaffected
firefox32 - affected

People

(Reporter: Margaret, Assigned: jryans)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

I'm not sure where this problem is happening, but right now I can only connect to Fennec Aurora, not Fennec Nightly (or my local m-c build). It just times out.

Is anyone else seeing this problem?

I'm using desktop Firefox Nightly.
It's caused by a regression in bug 859372. Not sure if Eddy wants to fix it here or there.
I offered to take a look at this, since Eddy doesn't currently have the Fennec workflow set up.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Keywords: regression
Here's a new test to ensure the debugger server is started, using the path that browser.js actually goes through.

I've verified this test failed initially, and now passes with the addition of part 2.
Attachment #8423425 - Flags: review?(margaret.leibovic)
Margaret, I needed to move a file (dbg-browser-actors.js) out of chrome/content and over to modules.  Does that seem fine to you?

Panos, can you review the rest of the work here, including changes to the content of |dbg-browser-actors.js| and the other DevTools files?
Attachment #8423429 - Flags: review?(past)
Attachment #8423429 - Flags: review?(margaret.leibovic)
Comment on attachment 8423429 [details] [diff] [review]
Part 2: Repair dbg-browser-actors on Fennec

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

Looks good to me and I bet metro and webapprt will need the same fixes.
Attachment #8423429 - Flags: review?(past) → review+
Comment on attachment 8423425 [details] [diff] [review]
Part 1: Test debug server startup on Fennec

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

That's one small step for our tests, one giant leap for devtools stability.
Thank you for doing this, this is great!
Comment on attachment 8423425 [details] [diff] [review]
Part 1: Test debug server startup on Fennec

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

Excellent! I love how simple this is :)
Attachment #8423425 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8423429 [details] [diff] [review]
Part 2: Repair dbg-browser-actors on Fennec

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

::: mobile/android/modules/moz.build
@@ +7,5 @@
>  EXTRA_JS_MODULES += [
>      'Accounts.jsm',
>      'AndroidLog.jsm',
>      'ContactService.jsm',
> +    'dbg-browser-actors.js',

Can we rename this file to match the other modules? Something like DebugBrowserActors.jsm?
Attachment #8423429 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #10)
> Comment on attachment 8423429 [details] [diff] [review]
> Part 2: Repair dbg-browser-actors on Fennec
> 
> Review of attachment 8423429 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/moz.build
> @@ +7,5 @@
> >  EXTRA_JS_MODULES += [
> >      'Accounts.jsm',
> >      'AndroidLog.jsm',
> >      'ContactService.jsm',
> > +    'dbg-browser-actors.js',
> 
> Can we rename this file to match the other modules? Something like
> DebugBrowserActors.jsm?

Well, it's not actually in the JSM format, so that's why I left the name as is.  It's now in the CommonJS format that the DevTools loader (and the Addon SDK) uses.

Is there a better folder to use perhaps?  Should I make a new one?  Mainly I just need to be able refer to it with a resource:// URL.
Flags: needinfo?(margaret.leibovic)
Updated test to clear pref on cleanup.
Attachment #8423425 - Attachment is obsolete: true
Attachment #8424039 - Flags: review+
I applied the second patch locally, and it lets the debug server start up.  I can connect to my device now, and use the inspector, for example.  I can set a breakpoint in browser.js, but I can't set a breakpoint in some aboutDevices.js code I am modifying.  My guess is the latter is unrelated -- I've heard of breakpoint flakiness from other folks -- so this appears to be a good fix.
Could you also export at least BrowserTabActor? Maybe it would make sense to export the other classes, in case they need to be extended by other actors.
(In reply to Nick Alexander :nalexander from comment #13)
> I can set a breakpoint in browser.js, but I can't set a breakpoint in some
> aboutDevices.js code I am modifying.  My guess is the latter is unrelated --
> I've heard of breakpoint flakiness from other folks -- so this appears to be
> a good fix.

Could you file a separate bug for this, please? Does it make a difference if you connect the debugger to the device after aboutDevice.js is already loaded in Fennec?
(In reply to Philipp Kewisch [:Fallen] from comment #14)
> Could you also export at least BrowserTabActor? Maybe it would make sense to
> export the other classes, in case they need to be extended by other actors.

From an API design POV it makes sense to only export things that are absolutely needed in order to limit the API surface. That said, we will of course expose anything that is demonstrably useful.
(In reply to Panos Astithas [:past] from comment #16)
> (In reply to Philipp Kewisch [:Fallen] from comment #14)
> > Could you also export at least BrowserTabActor? Maybe it would make sense to
> > export the other classes, in case they need to be extended by other actors.
> 
> From an API design POV it makes sense to only export things that are
> absolutely needed in order to limit the API surface. That said, we will of
> course expose anything that is demonstrably useful.

Generally I do agree. Thunderbird uses a different tab list implementation, but uses the same tab actors:
http://mxr.mozilla.org/comm-central/source/mail/components/devtools/modules/XULRootActor.js#152
(In reply to Philipp Kewisch [:Fallen] from comment #17)
> (In reply to Panos Astithas [:past] from comment #16)
> > (In reply to Philipp Kewisch [:Fallen] from comment #14)
> > > Could you also export at least BrowserTabActor? Maybe it would make sense to
> > > export the other classes, in case they need to be extended by other actors.
> > 
> > From an API design POV it makes sense to only export things that are
> > absolutely needed in order to limit the API surface. That said, we will of
> > course expose anything that is demonstrably useful.
> 
> Generally I do agree. Thunderbird uses a different tab list implementation,
> but uses the same tab actors:
> http://mxr.mozilla.org/comm-central/source/mail/components/devtools/modules/
> XULRootActor.js#152

Seems fine to export BrowserTabActor too then.  I'll update part 2 to do so before landing.
(In reply to J. Ryan Stinnett [:jryans] from comment #11)
> (In reply to :Margaret Leibovic from comment #10)
> > Comment on attachment 8423429 [details] [diff] [review]
> > Part 2: Repair dbg-browser-actors on Fennec
> > 
> > Review of attachment 8423429 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/modules/moz.build
> > @@ +7,5 @@
> > >  EXTRA_JS_MODULES += [
> > >      'Accounts.jsm',
> > >      'AndroidLog.jsm',
> > >      'ContactService.jsm',
> > > +    'dbg-browser-actors.js',
> > 
> > Can we rename this file to match the other modules? Something like
> > DebugBrowserActors.jsm?
> 
> Well, it's not actually in the JSM format, so that's why I left the name as
> is.  It's now in the CommonJS format that the DevTools loader (and the Addon
> SDK) uses.
>
> Is there a better folder to use perhaps?  Should I make a new one?  Mainly I
> just need to be able refer to it with a resource:// URL.

Ah, I didn't look closely enough. I don't think we need to make a new directory, since there aren't very many files in there right now. So your patch is fine :)
Flags: needinfo?(margaret.leibovic)
https://hg.mozilla.org/mozilla-central/rev/d6b3401d9d74
https://hg.mozilla.org/mozilla-central/rev/89aae014a99f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
With this being resolved fixed going to pass on tracking.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: