Alexandre Poirot [:ochameau] Reporter
	Description • 9 years ago

Bug 885982 broke adbhelper addon as it was using old TCPSocket js code.
The issue now is that there is no sane way to retrieve a TCPSocket instance for chrome code. You have to toggle the pref, which is unfortunate. Also, you need a document, whereas it isn't really necessary for chrome code.

So ideally, it would be great if we can get a TCPSocket instance from arbitrary chrome code.
Josh already suggested me some changes and I will submit a patch soon.

	apoirot
	Comment 1 • 9 years ago

Simple patch, allows me to easily fetch a TCPSocket instance from chrome docs
or JSM/XPCom...

But I had to hack on Navigator.
I wasn't able to find any possible way to define ShouldTCPSocketExist
within dom/network/TCPSocket.{h,cpp}.
I have issues to pull the symbol from TCPSocket.webidl.
  Func=mozilla::dom::ShouldTCPSocketExist
  Fails by trying to include mozilla/dom.h !?!!

  Func=ShouldTCPSocketExist
  Doesn't find the symbol

I tried by defining this function as static on the LegacyMozTCPSocket
class or not and end up with same issue.

	Josh Matthews [:jdm] Assignee
	Comment 2 • 9 years ago

Have you tested adding event listeners and handling events? I did originally try something like this and I remember having problems that I couldn't figure out how to resolve at the time.

	apoirot
	Comment 3 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b530cdbb2bc

	Alexandre Poirot [:ochameau] Reporter
	Comment 4 • 9 years ago

You are right, it looks like dom events like 'open' doesn't fire :/

Any suggestion? This is preventing gaia developers to use nightly to debug their phone,
hopefully we can address this regression in a reasonable timeframe!

	Josh Matthews [:jdm] Assignee
	Comment 5 • 9 years ago

I strongly recommend rewriting your code to use socket.jsm from comm-central.

	Alexandre Poirot [:ochameau] Reporter
	Comment 6 • 9 years ago

Oh wait, I found that GetOwner() is null.
It looks like I get more stuff working (at least events) when doing:
   if (NS_WARN_IF(!api.Init(mozilla::DOMEventTargetHelper::GetParentObject()))) {
instead of
    if (NS_WARN_IF(!api.Init(GetOwner()))) {

The addon doesn't work as-is yet, but at least events work.

	Alexandre Poirot [:ochameau] Reporter
	Comment 7 • 9 years ago

Ok. There is some subtle API diff with the previous JS version of TCPSocket.
But I got the addon to work again.
I'll submit the patches and you will tell me if that's reasonable.

That would be great to depend on this interface (that may become a standard? and is in c++),
rather than some external gecko-only module in js.

	Alexandre Poirot [:ochameau] Reporter
	Comment 8 • 9 years ago

And here is a fully working patch.
GetOwner() returns null as it tries to cast a window out of the global.
Using mozilla::DOMEventTargetHelper::GetParentObject() is just a way
to return the `aGlobal` passed in TCPSocket constructor.
LegacyMozTCPSocket saves aGlobal to a mGlobal and uses it for its jsapi.Init calls.

I'm still open for help for the Func= / ShouldTCPSocketExist thing.

	Josh Matthews [:jdm] Assignee
	Comment 10 • 9 years ago

Comment on attachment 8664845 [details] [diff] [review]
Expose TCPSocket to chrome contexts

I'm trying to write an automated test, so this can be put on hold.

	Josh Matthews [:jdm] Assignee
	Comment 11 • 9 years ago

Now with test.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 9 years ago

Comment on attachment 8664938 [details] [diff] [review]
Expose TCPSocket to chrome contexts

Please use GetOwnerGlobal, not GetParentObject, to get the relevant global. Then you should be able to remove the explicit qualifications and the one nasty cast, right?

Also, can this let us drop the override of GetParentObject we have on TcpSocket stuff?

You need to include 'mozilla::dom::' in your function name in the idl.

r=me

	Chris Lord [:cwiiis]
	Comment 13 • 9 years ago

When this bug is fixed, will this fix WebIDE for FirefoxOS devices? It's been broken for the last week...

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 14 • 9 years ago

(In reply to Chris Lord [:cwiiis] from comment #13)
> When this bug is fixed, will this fix WebIDE for FirefoxOS devices? It's
> been broken for the last week...

It's a piece of the puzzle at least.  We'll also need to update the ADB Helper add-on.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 15 • 9 years ago

Josh, can we get this landed soon?  AIUI, it's making things hard for the Gaia team since device connection is blocked by this issue.

	Josh Matthews [:jdm] Assignee
	Comment 16 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdc55e3e1a42

	Josh Matthews [:jdm] Assignee
	Comment 17 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=348e7558c986

	Josh Matthews [:jdm] Assignee
	Comment 18 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dad5e7102f74

	Josh Matthews [:jdm] Assignee
	Comment 19 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/37c89ad82a13066287f8fed5711a0442b9b391aa
Bug 1207090 - Expose TCPSocket to chrome contexts. r=bz

	Nigel Babu [:nigelb]
	Comment 20 • 9 years ago

This has caused hazards bustage. Also seen on one of the try pushes. Will back it out.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 9 years ago

Er, yes.  Need to locally root aGlobal in ShouldTCPSocketExist across the GetBool call, or do the preferences check after the last use of aGlobal.

	Pulsebot
	Comment 22 • 9 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f47a15c9c44b

	Josh Matthews [:jdm] Assignee
	Comment 23 • 9 years ago

Sorry, when I looked at the output log I focused on the "CalledProcessError: Command '['hg', 'log', '-r', '4e264d917bbacfad23327999e7e070af8fa86d90', '--template', '{node}']' returned non-zero exit status 255" and didn't notice the TEST-UNEXPECTED-FAIL line. My mistake.

	Josh Matthews [:jdm] Assignee
	Comment 24 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e146496aec0d0e33ae56e28781bf44e5ac5e4a
Bug 1207090 - Expose TCPSocket to chrome contexts. r=bz

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 25 • 9 years ago

[Tracking Requested - why for this release]: We'll want to be sure this goes in 43 as well.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 26 • 9 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 27 • 9 years ago

Comment on attachment 8664938 [details] [diff] [review]
Expose TCPSocket to chrome contexts

Approval Request Comment
[Feature/regressing bug #]: Bug 885982 changed the way TCPSocket is accessed in 43.
[User impact if declined]: ADB Helper (for DevTools connecting to devices) won't work without this fix.
[Describe test coverage new/current, TreeHerder]: on m-c, manually tested with new ADB Helper.
[Risks and why]: Low, exposes TCPSocket to chrome content, where it was before bug 885982.
[String/UUID change made/needed]: None

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 28 • 9 years ago

Comment on attachment 8664938 [details] [diff] [review]
Expose TCPSocket to chrome contexts

Fix for dev tools features, approved to uplift to aurora.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 29 • 9 years ago

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

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 30 • 9 years ago

Tracking this belatedly since it's a regression.