Closed Bug 1131997 Opened 9 years ago Closed 9 years ago

Adapt for Debugger Server code for changes in bug 1059308

Categories

(Thunderbird :: General, defect)

30 Branch
x86
macOS
defect
Not set
normal

Tracking

(thunderbird39 fixed, thunderbird40 fixed, thunderbird_esr38 unaffected)

RESOLVED FIXED
Thunderbird 40.0
Tracking Status
thunderbird39 --- fixed
thunderbird40 --- fixed
thunderbird_esr38 --- unaffected

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

Attachments

(1 file, 2 obsolete files)

We will get a nice window selector fairly soon. To do so we need to set DebuggerServer.allowChromeProcess in time. I've also set the discoverable property to true for the listener if wifi debugging is enabled, this makes it automatically show up in webide.
Attached patch Fix - v1 (obsolete) β€” β€” Splinter Review
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #8562749 - Flags: review?(neil)
Attachment #8562749 - Flags: review?(mconley)
Comment on attachment 8562749 [details] [diff] [review]
Fix - v1

Thanks for fixing this.

>     var port = Services.prefs.getIntPref(DEBUGGER_REMOTE_PORT);
We have constants for all of our debugger preferences, it would be nice to use them.

>+      // Expose this listener via wifi discovery, if enabled.
>+      if (Services.prefs.getBoolPref("devtools.remote.wifi.visible") &&
>+          !Services.prefs.getBoolPref("devtools.debugger.force-local")) {
>+        listener.discoverable = true;
>+      }
When DEBUGGER_FORCE_LOCAL changes we restart the listener; I think we should extend the code to deal with the wifi visibility change too (but don't restart if force local is set of course).
Attachment #8562749 - Flags: review?(neil) → feedback+
Attached patch Fix - v2 (obsolete) β€” β€” Splinter Review
Here is an updated patch. I haven't tested the Seamonkey bits since I don't have a compiled build, maybe you can do that for me. Do you think you could also review the Thunderbird bits to take some pressure off of Mike?
Attachment #8562749 - Attachment is obsolete: true
Attachment #8562749 - Flags: review?(mconley)
Attachment #8576826 - Flags: review?(neil)
Well it all looks well and good but what are the actual steps to test this?
The mentioned bug is more about adding that window selector icon in the toolbox, which should work with this patch. The changes in this bug make sure that chrome debugging ("Main Process") is available again. The wifi debugging is a bit of a piggyback, but if you want to test it set devtools.remote.wifi.visible in TB/SM and set devtools.remote.wifi.scan in Firefox and restart. In WebIDE you should then have Thunderbird show up in the devices list automatically or until I move forward on 1131990 at least the hostname.
(In reply to Philipp Kewisch from comment #5)
> The mentioned bug is more about adding that window selector icon in the
> toolbox, which should work with this patch. The changes in this bug make
> sure that chrome debugging ("Main Process") is available again.
So, apparently the command to rebuild the JS component changed from make libs to make misc, which meant that I wasn't seeing any changes. Now that I've got that sorted out, I can turn on and use the window selector icon, which is very nice, and also debug the main process again.

> The wifi
> debugging is a bit of a piggyback, but if you want to test it set
> devtools.remote.wifi.visible in TB/SM and set devtools.remote.wifi.scan in
> Firefox and restart. In WebIDE you should then have Thunderbird show up in
> the devices list automatically or until I move forward on 1131990 at least
> the hostname.
I'm not seeing anything... any idea where I could start troubleshooting?
(In reply to neil@parkwaycc.co.uk from comment #6)
> 
> > The wifi
> > debugging is a bit of a piggyback, but if you want to test it set
> > devtools.remote.wifi.visible in TB/SM and set devtools.remote.wifi.scan in
> > Firefox and restart. In WebIDE you should then have Thunderbird show up in
> > the devices list automatically or until I move forward on 1131990 at least
> > the hostname.
> I'm not seeing anything... any idea where I could start troubleshooting?

Did you also restart Thunderbird? I think at least on mac it has to get the firewall privs so it can accept incoming connections. You can enable devtools.debugger.log, if that doesn't give you enough information I'd suggest asking in #devtools.
Comment on attachment 8576826 [details] [diff] [review]
Fix - v2

>+      DebuggerServer.allowChromeProcess = true;
>       DebuggerServer.init();
One thing I happened to notice is that init() resets allowChromeProcess so you need to set that afterwards. (Haven't had a chance to read your comment yet sorry.)
(In reply to neil@parkwaycc.co.uk from comment #8)

> One thing I happened to notice is that init() resets allowChromeProcess so
> you need to set that afterwards. (Haven't had a chance to read your comment
> yet sorry.)

Could you point me to where thats happening? I checked http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/devtools/server/main.js#177 and there is no sign of allowChromeProcess being reset.
Comment on attachment 8576826 [details] [diff] [review]
Fix - v2

I don't know much about the wifi story, but allowChromeProcess looks good.
Note that DebuggerServer.init shouldn't reset allowChromeProcess,
but what happened while working on this is that if you only set
allowChromeProcess=false within the `if (!DebuggerServer.initialized)`;
The flag won't be set if the debugger server is started before by some other code.
Like tests... (I had broken tests because of this).

So I would just suggest setting allocChromeProcess after the `if (!DebuggerServer.initialized)` block.
Attachment #8576826 - Flags: feedback+
(In reply to Philipp Kewisch from comment #9)
> (In reply to comment #8)
> > One thing I happened to notice is that init() resets allowChromeProcess so
> > you need to set that afterwards. (Haven't had a chance to read your comment
> > yet sorry.)
> Could you point me to where thats happening? I checked
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/devtools/server/
> main.js#177 and there is no sign of allowChromeProcess being reset.
With your patch as shown I couldn't access the chrome process, but if I switched the two lines then it worked. (And in regards to Alex's comment, it was still inside the if block.)
Attached patch Fix - v3 β€” β€” Splinter Review
Ah now I see what you mean. Sounds like a plan, how is this?
Attachment #8576826 - Attachment is obsolete: true
Attachment #8576826 - Flags: review?(neil)
Attachment #8585087 - Flags: review?(neil)
Comment on attachment 8576826 [details] [diff] [review]
Fix - v2

OK, so I updated both my SeaMonkey and Firefox trees again and now this patch works, so if you don't mind I have a slight preference for this version.
Attachment #8576826 - Attachment is obsolete: false
Attachment #8576826 - Flags: review+
Attachment #8585087 - Attachment is obsolete: true
Attachment #8585087 - Flags: review?(neil)
Now I am confused. What about Alex's comment about what happens if something else initialized the debugger? In the end I don't mind either way, if you still think we should go with v2, please reply and set checkin-needed.
Flags: needinfo?(neil)
Comment on attachment 8585087 [details] [diff] [review]
Fix - v3

(In reply to Philipp Kewisch from comment #15)
> What about Alex's comment about what happens if something
> else initialized the debugger?

Sorry, I misunderstood it first time around, and now I see what he's on about, so yes, v3 is fine.
Attachment #8585087 - Attachment is obsolete: false
Flags: needinfo?(neil)
Attachment #8585087 - Flags: review+
Attachment #8576826 - Attachment is obsolete: true
Thanks for taking a look!
Keywords: checkin-needed
Comment on attachment 8585087 [details] [diff] [review]
Fix - v3

[Approval Request Comment]
Regression caused by (bug #): bug 1059308 
User impact if declined: Debugging Thunderbird 39 will not be possible
Testing completed (on c-c, etc.): local testing
Risk to taking this patch (and alternatives if risky): In the worst case nonfunctioning debugger, but given its broken already that wouldn't be much of a change.
Attachment #8585087 - Flags: approval-comm-aurora?
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
Comment on attachment 8585087 [details] [diff] [review]
Fix - v3

http://hg.mozilla.org/releases/comm-beta/rev/0aa9c5151a37
Attachment #8585087 - Flags: approval-comm-aurora? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.