562320 - Change Gopher C++ for e10s

Status: RESOLVED DUPLICATE of bug 590682

(Core :: Networking, defect)

Description

[Cameron Kaiser [:spectre]]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.8.1.24) Gecko/20100305 Camino/1.6.11 (like Firefox/2.0.0.24)
Build Identifier: 

Per discussion in bug 388195, this is for reimplementation of Gopher in core, but this time in JavaScript instead of C++. See comments 100 through 103.

Reproducible: Always

Comment 1

[Jason Duell]

Ben, please make sure to ping Cameron when we're got reverse CPOWs or whatever else might make JS gopher work.  (Do we have a bug open for reverse CPOW?)

Comment 2

[Cameron Kaiser [:spectre]]

Since Joe recommended starting with a test suite, it seems most logical to me to do such a thing through the xpcshell test harness and create a small gopher server similar to httpd.js to test against. Would that be the best way? I don't think this would be a good fit for mochitest since it seems HTTP-centric.

Comment 3

[Jason Duell]

> xpcshell test harness and create a small gopher server similar to httpd.js

Yes, that sounds like a good plan to me.

Comment 4

[Cameron Kaiser [:spectre]]

Attachment: Work in progress, Gopher e10s

WIP on Gopher in e10s. The attached test case does work for 1.9.2 but I must be missing something because xpcshell-test just freezes up waiting for a signal I must not be sending. What am I missing here? This is a combination of both the existing code for HttpChannel* and FtpChannel*.

Comment 5

[Cameron Kaiser [:spectre]]

Attachment: WIP attempt 2 (in line with 536289)

WIP 2. Updated to review of 536289 since this is based on it. Still can't get unit test to work: it hangs when it asks the ioService for a channel. What did I do wrong here?

Comment 6

[Josh Matthews [:jdm]]

Problem #1: You're using the RefcountHitZero method (which is pretty self-describing), but then calling Release() in DeallocPGopherChannel.  That's wrong; you should be deleting the pointer instead.  As it stands right now, you're still not releasing the IPDL actor at any point, so that code will never run.

Problem #2: You haven't changed the nsGopherHandler::NewProxiedChannel code to hand back a GopherChannelChild in the content process, so none of the IPDL work ever happens.  This is the cause of the ioService hang.

Comment 7

[Cameron Kaiser [:spectre]]

I think I get it now. Respin coming up. Thanks!

Comment 8

[Cameron Kaiser [:spectre]]

Attachment: Working patch with simple unit test

With Josh's suggested changes. This passes the attached unit test.

Comment 9

[Cameron Kaiser [:spectre]]

Attachment: Revised working patch with simple unit test

Whoops, repaired reference leak. Also made similar nitfixes requested in 536289.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We don't support gopher anymore, do we?

Comment 11

[Dan Mellem]

See, for example, bug 388195 comment 67 and bug 388195 comment 100+. It may be added back to core if it's in a more secure language (i.e., JavaScript) and the interfaces exist in electrolysis (e10s).

Comment 12

[Jason Duell]

We hope to support Gopher in JS, but we're never going to implement it in C++, and that code's been removed from non-e10s.

Comment 13

[Dan Mellem]

I think you misunderstood this bug. This is to fix the C++ so that Gopher can be implemented in JavaScript. There are some things that need to be fixed for the electrolysis version that are blocking the conversion to JS.

Comment 14

[Jason Duell]

Right you are!

Comment 15

[Cameron Kaiser [:spectre]]

Actually, I would think that bug 590682 (especially Honza's patch, if I understand it correctly) should cover this case for any protocol, including Gopher.

Comment 17

[Justin Dolske [:Dolske]]

Comment on attachment 450568 [details] [diff] [review]
Revised working patch with simple unit test

Clearing review request, since this has been dupe'd to another bug. Please reopen or file a new bug if this issue is still somehow no covered by the bug it's been dupe'd to.