Change Gopher C++ for e10s

RESOLVED DUPLICATE of bug 590682

Status

()

Core
Networking
RESOLVED DUPLICATE of bug 590682
8 years ago
6 years ago

People

(Reporter: spectre, Assigned: spectre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

8 years ago
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

Updated

8 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?)
Assignee: nobody → spectre
(Assignee)

Comment 2

8 years ago
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.
> xpcshell test harness and create a small gopher server similar to httpd.js

Yes, that sounds like a good plan to me.
(Assignee)

Updated

8 years ago
Summary: Reimplement Gopher in JavaScript → Change Gopher C++ for e10s
(Assignee)

Comment 4

8 years ago
Created attachment 449575 [details] [diff] [review]
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*.
Attachment #449575 - Flags: feedback?
(Assignee)

Updated

8 years ago
Attachment #449575 - Attachment is obsolete: true
Attachment #449575 - Flags: feedback?
(Assignee)

Comment 5

8 years ago
Created attachment 450296 [details] [diff] [review]
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?
Attachment #450296 - Flags: feedback?
Attachment #450296 - Flags: feedback? → feedback?(jduell.mcbugs)

Comment 6

8 years ago
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.

Updated

8 years ago
Attachment #450296 - Flags: feedback?(jduell.mcbugs)
(Assignee)

Comment 7

8 years ago
I think I get it now. Respin coming up. Thanks!
(Assignee)

Comment 8

8 years ago
Created attachment 450460 [details] [diff] [review]
Working patch with simple unit test

With Josh's suggested changes. This passes the attached unit test.
Attachment #450296 - Attachment is obsolete: true
Attachment #450460 - Flags: review?
(Assignee)

Comment 9

8 years ago
Created attachment 450568 [details] [diff] [review]
Revised working patch with simple unit test

Whoops, repaired reference leak. Also made similar nitfixes requested in 536289.
Attachment #450460 - Attachment is obsolete: true
Attachment #450568 - Flags: review?
Attachment #450460 - Flags: review?
We don't support gopher anymore, do we?

Comment 11

6 years ago
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).
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX

Comment 13

6 years ago
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Right you are!
(Assignee)

Comment 15

6 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 590682
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.
Attachment #450568 - Flags: review?
You need to log in before you can comment on or make changes to this bug.