1060464 - --jsdebugger switch in mochitests crashes immediately because of non-local connections (debugging profile gets the env var telling us to crash, don't get alternative prefs so we don't hit the network so much)

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Tomasz Kołodziejski [:tomasz]]

I tried

mach mochitest-chrome tests/chrome/test_findbar.xul --jsdebugger

but the browser toolbox (--jsdebugger) window immediately crashes with

3 INFO FATAL ERROR: Non-local network connections are disabled and a connection attempt to tiles.services.mozilla.com (54.186.213.109) was made.
4 INFO You should only access hostnames available via the test networking proxy (if running mochitests) or from a test-specific httpd.js server (if running xpcshell tests). Browser services should be disabled or redirected to a local server.

I talked to :Gijs and it looks like it's a regression introduced in bug 1042876.

Comment 1

[:Gijs (he/him)]

Ed, can we add dummy prefs for the tiles thing you added in bug 1042876 so that this doesn't happen? (well, assuming that that fixes this)

I'm not sure why only reftests were fixed.

Comment 2

[Tomasz Kołodziejski [:tomasz]]

I can confirm that dropping

https://hg.mozilla.org/integration/fx-team/file/8f49394f8069/browser/app/profile/firefox.js#l1539
and
https://hg.mozilla.org/integration/fx-team/file/8f49394f8069/browser/app/profile/firefox.js#l1542

"fixes" my issue.

Comment 3

[Ed Lee :Mardak]

Where do these preferences need to be set?

They've been set in these files:
- addon-sdk/source/python-lib/cuddlefish/prefs.py
- layout/tools/reftest/runreftest.py
- testing/profiles/prefs_general.js
- testing/talos/talos.json -> talos/PerfConfigurator.py

Comment 4

[:Gijs (he/him)]

(In reply to Ed Lee :Mardak from comment #3)
> Where do these preferences need to be set?
> 
> They've been set in these files:
> - addon-sdk/source/python-lib/cuddlefish/prefs.py
> - layout/tools/reftest/runreftest.py
> - testing/profiles/prefs_general.js
> - testing/talos/talos.json -> talos/PerfConfigurator.py

Oh! I'm sorry, these weren't in the patch on the bug (v2)... that then sounds like these aren't being used for the debugging profile. We should probably either fix the debugger code to use those prefs for its new profile (sounds hard) or fix the mochitest "die remote access die" code to not run against the debugger process... which actually also sounds hard, because it looks like it's triggered by an env var ("MOZ_DISABLE_NONLOCAL_CONNECTIONS"), which will be propagated to the debugging process started by the mainline firefox process...

Comment 5

[:Gijs (he/him)]

Dave, could we copy the prefs.js file from the main profile to the test profile when creating a debugging profile? That'd basically fix this issue immediately, and potentially have some benefits in terms of the prefs you'd have in your main profile regarding debugging and devtools and so on would persist in the new chrome debugging profile...

Comment 6

[Dave Camp (:dcamp)]

Yeah that seems reasonable.

Comment 7

[:Gijs (he/him)]

Attachment: fix the --jsdebugger flag when running tests, also fixes bug 1060493,

The prefs file sync writing hack is icky, but it works and the difference with nsIFile.copyTo here is probably slim-to-none, nevermind the difference with calling createProfile, which is what we did before. I don't feel comfortable also making all of this async in this changeset, so we'll leave that for another time, if that's OK...

Comment 8

[:Gijs (he/him)]

Attachment: fix the --jsdebugger flag when running tests, also fixes bug 1060493,

Fixed up octal syntax, got rid of one clone() - left the other for clarity...

Comment 9

[:Gijs (he/him)]

Comment on attachment 8481709 [details] [diff] [review]
fix the --jsdebugger flag when running tests, also fixes bug 1060493,

23:16	<dcamp>	Gijs: away from my computer atm, but you have my r+ if you have bsmedberg's
23:17	<dcamp>	Gijs: that patch was outside of my comfort zone and I should have passed it off, sorry
23:17	<dcamp>	Gijs: but if it works, it can't be worse than what I'd there

Comment 10

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/c1319f301eb0

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/19ad1a4a9bb2 for devtools failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=47267918&tree=Fx-Team

Comment 12

[:Gijs (he/him)]

Attachment: fix the --jsdebugger flag when running tests, also fixes bug 1060493,

Sigh - I shouldn't have assumed that the private properties were only in use in the jsm, clearly... I've adapted the test, and sent this to try: https://tbpl.mozilla.org/?tree=Try&rev=8697718dc0a0

Comment 13

[:Gijs (he/him)]

Comment on attachment 8483738 [details] [diff] [review]
fix the --jsdebugger flag when running tests, also fixes bug 1060493,

Carrying over review...

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Did this reland in fx-team or not? I'm still getting the FATAL ERROR message on the latest fx-team. Did we just forget to remove [fixed-in-fx-team]?

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Checked this in as https://hg.mozilla.org/integration/fx-team/rev/303c2817e129

If this fails again, Gijs, can you make sure the try run also tests against mochitest-dt in addition to mochitest-bc?

Comment 17

[:Gijs (he/him)]

(In reply to Wes Kocher (:KWierso) from comment #16)
> Checked this in as
> https://hg.mozilla.org/integration/fx-team/rev/303c2817e129
> 
> If this fails again, Gijs, can you make sure the try run also tests against
> mochitest-dt in addition to mochitest-bc?

*bangs head against wall*

Sigh. Good thing the local testing seems to have been representative, as fx-team looks green. Thanks for checking this in! :-)

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/303c2817e129

Comment 19

[Tomasz Kołodziejski [:tomasz]]

Toolbox crashes after this change with:

System JS : ERROR resource:///modules/devtools/ToolboxProcess.jsm:162 - ReferenceError: Cr is not defined

It looks like you've forgotten:

const Cr = Components.results;

Repro:
0. make clean profile
1. mach run --jsdebugger
2. close everything
3. mach run --jsdebugger
4. crash

Comment 20

[Girish Sharma [:Optimizer]]

(This shows that we really need a test for this :) )

Comment 21

[Tomasz Kołodziejski [:tomasz]]

Attachment: forgotten-cr.patch

This fixes the issue for me.

Comment 22

[:Gijs (he/him)]

Comment on attachment 8484487 [details] [diff] [review]
forgotten-cr.patch

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

rs=me

Comment 23

[Tomasz Kołodziejski [:tomasz]]

I'm not sure what rs=me means. BTW, would you submit it or should I checkin-needed' it?

Comment 24

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/ad6298e44579


(In reply to Tomasz Kołodziejski [:tomasz] from comment #23)
> I'm not sure what rs=me means. BTW, would you submit it or should I
> checkin-needed' it?

rs=rubber-stamp

I just landed this (see above).

(In reply to Girish Sharma [:Optimizer] from comment #20)
> (This shows that we really need a test for this :) )

We have a test; the issue Tomasz found only shows up the second time you launch the browser debugger for the same profile. Feel free to file a followup bug to test that.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ad6298e44579

Comment 27

[:Gijs (he/him)]

Nick, is this worth uplifting to aurora (considering this broke slightly before 34 branched, and 34's aurora branch is young yet), or shall we just let it ride the trains?

Comment 28

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Yes, let's uplift.

Comment 29

[:Gijs (he/him)]

Attachment: Combined patch for Aurora

Comment 30

[:Gijs (he/him)]

Comment on attachment 8485807 [details] [diff] [review]
Combined patch for Aurora

Approval Request Comment
[Feature/regressing bug #]: long-standing bug became acute when we landed prefs which caused network connections on startup on new profiles (bug 1042876)
[User impact if declined]: can't really use jsdebugger to debug mochitests
[Describe test coverage new/current, TBPL]: some automated test coverage, but could be better...
[Risks and why]: fundamental aspect of how we run the jsdebugger changed, which is inherently risky - but equally, this gets used by frontend and devtools folks pretty heavily, and we would have noticed by now if it wasn't properly fixed (but is definitely broken on 34 right now), and 34 is still very young
[String/UUID change made/needed]: none

Comment 31

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8485807 [details] [diff] [review]
Combined patch for Aurora

It is still early in the Aurora 34 cycle. Taking this to facilitate dev work. Aurora+

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f98ecc53c842