Expose WebIDL TCPSocket to chrome code

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: ochameau, Assigned: jdm)

Tracking

({regression})

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 unaffected, firefox43+ fixed, firefox44 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

4 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.
Reporter

Updated

4 years ago
Depends on: 885982

Comment 1

4 years ago
Posted patch patch v1 (obsolete) — Splinter Review
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.
Attachment #8664164 - Flags: review?(josh)
Assignee

Comment 2

4 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.
Reporter

Comment 4

4 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!
Assignee

Comment 5

4 years ago
I strongly recommend rewriting your code to use socket.jsm from comm-central.
Reporter

Comment 6

4 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.
Reporter

Comment 7

4 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.
Reporter

Comment 8

4 years ago
Posted patch patch v2 (obsolete) — Splinter Review
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.
Attachment #8664164 - Attachment is obsolete: true
Attachment #8664164 - Flags: review?(josh)
Attachment #8664268 - Flags: review?(josh)
Assignee

Comment 9

4 years ago
Attachment #8664845 - Flags: review?(bzbarsky)
Assignee

Updated

4 years ago
Attachment #8664268 - Attachment is obsolete: true
Attachment #8664268 - Flags: review?(josh)
Assignee

Updated

4 years ago
Assignee: poirot.alex → josh
Status: NEW → ASSIGNED
Assignee

Comment 10

4 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.
Attachment #8664845 - Flags: review?(bzbarsky)
Assignee

Comment 11

4 years ago
Now with test.
Attachment #8664938 - Flags: review?(bzbarsky)
Assignee

Updated

4 years ago
Attachment #8664845 - Attachment is obsolete: true
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
Attachment #8664938 - Flags: review?(bzbarsky) → review+
When this bug is fixed, will this fix WebIDE for FirefoxOS devices? It's been broken for the last week...
(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.
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.
Flags: needinfo?(josh)
This has caused hazards bustage. Also seen on one of the try pushes. Will back it out.
Er, yes.  Need to locally root aGlobal in ShouldTCPSocketExist across the GetBool call, or do the preferences check after the last use of aGlobal.
Assignee

Comment 23

4 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.
Flags: needinfo?(josh)
[Tracking Requested - why for this release]: We'll want to be sure this goes in 43 as well.
https://hg.mozilla.org/mozilla-central/rev/a8e146496aec
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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
Attachment #8664938 - Flags: approval-mozilla-aurora?
Comment on attachment 8664938 [details] [diff] [review]
Expose TCPSocket to chrome contexts

Fix for dev tools features, approved to uplift to aurora.
Attachment #8664938 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking this belatedly since it's a regression.
You need to log in before you can comment on or make changes to this bug.