Closed Bug 1095484 Opened 6 years ago Closed 4 years ago

[e10s] ChatZilla add-on doesn't recognizing irc url in e10s mode

Categories

(Other Applications :: ChatZilla, defect)

defect
Not set
major

Tracking

(e10s+)

RESOLVED FIXED
Tracking Status
e10s + ---

People

(Reporter: Swarnava, Assigned: bugzilla-mozilla-20000923)

References

()

Details

(Keywords: addon-compat, Whiteboard: [amo-author-notified] triaged)

Attachments

(2 files, 5 obsolete files)

chatzilla is working when i trying to open it from Tools>Chatzilla.

but when i am tryting to open chatzilla by going irc://moznet/sumo , its asking me to use mibbit or any other irc messenger.

Expected Behavior:

It should open chatzilla directly
Thanks for reporting this bug, Swarnava. As Gijs noted in bug 1066623 comment 24, this addon bug is probably related to bug 940206.
Blocks: e10s-addons
Depends on: 940206
Adding to e10s backlog.
tracking-e10s: --- → +
This is still not working correctly. Notified via AMO.
tracking-e10s: + → ---
Whiteboard: [amo-author-notified]
The opinion has been that this is a core bug and not an add-on bug, per bug 1176646 comment 12 in particular, but details are hard to come by and nobody has investigated alternative solutions AFAIK. I'm setting the dependencies based on that analysis.
Depends on: 1175056
No longer depends on: 940206
Duplicate of this bug: 1176646
Attached patch WIP patch (obsolete) — Splinter Review
Assignee: rginda → blassey.bugs
(In reply to James Ross from comment #4)
> The opinion has been that this is a core bug and not an add-on bug, per bug
> 1176646 comment 12 in particular, but details are hard to come by and nobody
> has investigated alternative solutions AFAIK. I'm setting the dependencies
> based on that analysis.

Perhaps the title should be changed.  See discussion in bug 1176646, and also bug 940206 which is for a similar issue in web protocol handlers.  Perhaps something like "Custom protocol handlers only registered for chrome process"?
I realize that this bug currently does block e10s-addons, but haven't seen much movement on this for several months. It affects my extension as well (FireBible) and I believe several others too. Is this issue likely to be fixed by when e10s becomes default, or, at all?
Attachment #8653571 - Attachment is obsolete: true
Brian, this looks to me to be a dupe of bug 1190018, which is fixed. Does your addon work with the current nightly?
Flags: needinfo?(infernalproteus)
Brad, unfortunately it does not work in the latest Nightly either - I tried with 45.0a1 (2015-11-30), 32-bit, Windows 7. 

I tried both my extension and Chatzilla as well. In the case of the latter, "irc://mozilla.org" works just fine when e10s is turned off, it opens the Chatzilla window; when on, it asks me to choose an external program.

My extension works fine when e10s is off, when on, I get "Firefox doesn't know how to open this address, because one of the following protocols (bible) isn't associated with any program or is not allowed in this context."

Appreciate you looking into this - if you want to check yourself, testing with Chatzilla is the easiest by just typing "irc://mozilla.org" into the address bar. Thanks!
Flags: needinfo?(infernalproteus)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #9)
> Brian, this looks to me to be a dupe of bug 1190018, which is fixed.

bug 1190018 fixed navigator.registerProtocolHandler, which is the web-exposed thing that lets websites register as protocol handlers.

Both CZ and, presumably, Brian's add-on, use XPCOM to register themselves as protocol handlers. I expect that information doesn't apply to links in the child process for some reason. I think we should auto-register a forwarding handle for all protocols registered in the parent process into the child process, that passes off the navigation to the parent process. That'll be the most backwards-compatible and shouldn't be toooo hard.
I can confirm that protocol handlers registered in chrome, explicitly in an XPCOM component in my test, don't work in the latest nightly.  Or rather they do work, just not in the content process where they are mostly needed.
Assignee: blassey.bugs → mrbkap
I have a plan that I'll try to implement tomorrow. It's a little sketchy, but the idea is to detect when there's an nsIProtocolHandler in the parent process, cancel the load in the child and restart it again in the parent.

In the meantime, it'd probably be pretty easy to make chatzilla register its protocol handler in the child (with loadProcessScript) and a simple frame script to tell the parent to open the chatzilla window.
This affects OverbiteFF also, so I'd be very interested in this approach. It certainly would simplify the conversion enormously.
Blake, I noticed that the behavior in the latest nightly is still the same, did you have any time to implement the proposed plan?
Flags: needinfo?(mrbkap)
I have a partial patch that almost works. I should have a fix here tomorrow.
Flags: needinfo?(mrbkap)
Attached patch Possible patch v1 (obsolete) — Splinter Review
In order to make this work generally, I needed some more information on protocol handlers. The patch handles this case way later in e10s than otherwise -- we do the redirection in the external protocol handler. This isn't ideal, but it seems to work.

The additional bit of information I needed was for E10SUtils.canLoadURIInProcess: it needs to know what URIs it can forward to the child for general URIs. I hope this approach is OK.  Unfortunately, it has to be a whitelist, otherwise older extensions will do the wrong thing (which is infinitely loading their custom protocols and bouncing back and forth between the child and parent).

Bill, would you mind reviewing this for the approach and then I'll get a separate necko review?
Attachment #8720406 - Flags: review?(wmccloskey)
If I read this correctly, as long as an older extension implementing an XPCOM protocol handler doesn't set CAN_LOAD_IN_CHILD, it should "just work" by loading it in the parent process as in non-e10s. Is my understanding correct?

If so, I think that should be a perfect bridging strategy.
Flags: needinfo?(mrbkap)
(In reply to Cameron Kaiser [:spectre] from comment #19)
> If I read this correctly, as long as an older extension implementing an
> XPCOM protocol handler doesn't set CAN_LOAD_IN_CHILD, it should "just work"
> by loading it in the parent process as in non-e10s. Is my understanding
> correct?

Yes.
Flags: needinfo?(mrbkap)
Comment on attachment 8720406 [details] [diff] [review]
Possible patch v1

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

I'm trying to figure out why you need the code in ContentParent.cpp. My understanding of how this should work is as follows (based on our brief IRC conversation):

1. Someone clicks on an irc: link in the content process.
2. We use the external protocol handler to generate the URI, which returns a standard URL.
3. We'll go through this path, which is how we normally switch to the chrome process for navigations:
http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10207
4. That will eventually call into E10SUtils, which will again get the external protocol handler in the default: case.
5. The external protocol handler doesn't have the CAN_LOAD_IN_CHILD flag, so we should redirect the load to the parent.

I'm probably missing something, though, since I know very little about this area. My main concern in avoiding the ContentParent.cpp code is that it seems like it causes us to open the link in a new tab, which is maybe less desirable than using the same tab, which I think is our current behavior?

::: browser/modules/E10SUtils.jsm
@@ +56,5 @@
> +        // about URIs store additional information in the about module.
> +        let url = Services.io.newURI(aURL, null, null);
> +        let module = getAboutModule(url);
> +        // If the module doesn't exist then an error page will be loading, that
> +        // should be ok to load in either process

Can you add a period?
Comment on attachment 8720406 [details] [diff] [review]
Possible patch v1

Blake ran into some bugs so I'm cancelling for now.
Attachment #8720406 - Flags: review?(wmccloskey)
In particular, the patch, as I attached it, actually skips my code in ContentParent because I didn't add CAN_LOAD_IN_CHILD to nsExternalProtocolHandler and redirects (properly) to the parent. This mostly works, except that web app handlers (such as using gmail for mailto: links) don't load in the child process anymore (and mess up the history for the tab).

If I add CAN_LOAD_IN_CHILD to nsExternalProtocolHandler, then things actually work, with one exception:

in both cases, loading the irc: link in the same tab messes up the history for the tab. I get a grayed-out back button, but clicking my "back" mouse button goes back two pages. I'll debug that problem since it exists in both approaches. I'm leaning towards adding CAN_LOAD_IN_CHILD to nsExternalProtocolHandler since it's a bit safer but if someone feels strongly, I can pursue the first approach instead.
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #23)

> in both cases, loading the irc: link in the same tab messes up the history
> for the tab.

Are you testing just irc: links for ChatZilla or both irc: and ircs: links for it? I only just noticed that nobody has mentioned ircs: in the comments for this bug yet.
Flags: needinfo?(mrbkap)
(In reply to Mardeg from comment #24)
> Are you testing just irc: links for ChatZilla or both irc: and ircs: links
> for it? I only just noticed that nobody has mentioned ircs: in the comments
> for this bug yet.

I haven't specifically tested ircs: links (I'll make sure to do so on Monday), but the changes in this bug should apply equally for any protocol handler exposed via the component manager and "@mozilla.org/network/protocol;1?name=..."
Flags: needinfo?(mrbkap)
Attached patch Patch v1.1 (obsolete) — Splinter Review
After debugging the history bug, we figured out that it's because the irc: (and ircs:) protocol handlers don't return any content, which confuses the history-restoring code, leaving the tab in an inconsistent state (we'd actually want to navigate *back* to the page that tried to load the no-content URL, which would mean we'd have to switch tab remoteness again, etc.). It's easier to just load the external protocol handler in a new tab.

Chatzilla can also explicitly register a protocol handler in the content process via a process script, send a message to the parent and avoid this user experience. In order to do the new-tab behavior, I made the external protocol handler CAN_LOAD_IN_CHILD so I could override the behavior directly in ContentParent.
Attachment #8725482 - Flags: review?(wmccloskey)
Attachment #8720406 - Attachment is obsolete: true
A custom protocol handler may, as you found, load no content.  It may do nothing at all in the browser, for example just downloading a file.  Won't this approach create an unnecessary blank tab in those cases?
Just FYI: if the ChatZilla irc/ircs protocol handler has broken any of the documented requirements of a protocol handler, we'd be happy to fix it to avoid any unnecessary work elsewhere.
(In reply to Ian Nartowicz from comment #28)
> A custom protocol handler may, as you found, load no content.  It may do
> nothing at all in the browser, for example just downloading a file.  Won't
> this approach create an unnecessary blank tab in those cases?

It will. I guess we could try to notice that and close the new tab/window, but I'd rather do that in a separate bug. The ChatZilla protocol handler isn't violating any existing contracts, but the existing contracts definitely didn't have e10s in mind when they were created. We simply don't have enough information about what's going on until way too late in the process.
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #30)
> definitely didn't have e10s in mind when they were created. We simply don't
> have enough information about what's going on until way too late in the
> process.

I'll defer to your better knowledge, but I do see the flag URI_DOES_NOT_RETURN_DATA is set on the IRCProtocolHandler's protocolFlags and it seems like a good flag for this. Is this still too far down the line to prevent opening a new tab or whatever?
Ah, I forgot about that. We can use that. I'll do that as a separate patch tomorrow on top of the one attached here.
Attachment #8725482 - Flags: review?(wmccloskey) → review+
Byron, can you add tracking-e10s to this product/component?
Flags: needinfo?(glob)
tacking-e10s added to the chatzilla component.
Flags: needinfo?(glob)
Sorry to be a bother, but can I get an update on this fix? The latest nightly still exhibits the same problem. Concerned about whether this issue will be fixed at all. Thanks!
Flags: needinfo?(mrbkap)
Comment on attachment 8725482 [details] [diff] [review]
Patch v1.1

After talking to sicking, billm, and blassey about this patch, we decided not to take it. This patch does exactly enough to make Chatzilla's IRC protocol work and probably zero other extension protocol handlers (not to mention the problems with the NO_CONTENT protocol flags that I was working around). In addition, this is something that can easily be implemented better by the extension itself.

It's probably worth it to break the CAN_LOAD_IN_CHILD flag (and related E10SUtils.jsm changes) into their own bug, since right now I believe it's impossible for an extension to implement a protocol handler that will load in the parent process when the load is initiated from the child, even if it's registered in the child, but that's explicitly not needed here.

I also started working on a JS-implemented addon shim for this. Doing so would have been (I think) more palatable for some, as the shims are there for helping extensions transition to e10s. However, Chatzilla is already marked as being multiprocessCompatible and therefore doesn't get shims anyway. Also, my prototype shim (which I'll attach here for posterity) had to guess at a ton of stuff (like default port) and was, again, basically only going to work for Chatzilla. Anything more complicated than its BogusChannel would have required more changes.

In order to prove that this change to Chatzilla was minimal, please see [1]. I have some more specific notes in the commit referenced there, but most of the patch was code movement and adding an XPCOM startup observer. 

[1] https://github.com/mrbkap/chatzilla/commit/399833bdc69c5566d501438f3144d1cc8e5da067
Attachment #8725482 - Attachment is obsolete: true
Flags: needinfo?(mrbkap)
Attachment #8725482 - Flags: review+
Attached patch abandoned wip (obsolete) — Splinter Review
Here's the WIP shim that I was working on. It's pretty rough (there's a bunch of stuff that would have needed to be cleaned up) and I never managed to actually test enough to make sure there weren't any typos in the newly-added shim code. This didn't apply to Chatzilla because it doesn't explicitly call registerFactory (it uses jar.mn/chrome.manifest to do register the protocol handlers) and because it opts out of shims.
See Also: → 590682
Well, this is all very disappointing. Months and years to fix it, and at the last minute (relatively) deciding not to and pushing all the work on to add-ons. :(

I'll try and find the time to look at the changes Blake, but there's a good chance it won't be done (i.e. released) before the next Firefox cycle.
Hi James,  Sorry for the changes.  If it helps - here are the details on the add-ons/e10s plan... 

Beta 49 we're testing to make sure we don't see regressions against Firefox performance & stability with webextension add-ons and a test group of 8 MPC:true add-ons. If that goes well, the next step would be to test all MPC:True in Beta 50 (Beta sept 12/Release Nov 7).  Of course if we see a lot of impact - we will delay going to Release in 50 with MPC:true and determine the cause.   

there's a spreadsheet timeline I'll update as we get results from Beta experiments.
Whiteboard: [amo-author-notified] → [amo-author-notified] triaged
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: mrbkap → bugzilla-mozilla-20000923
Severity: normal → major
Version: unspecified → Trunk
Attached file 1095484-patch1.hg
Attached patch 1095484-patch1.diff (obsolete) — Splinter Review
This is the diff from trunk, while the other attachment is the HG bundle of two commits (one is Blake's and one is mine).
Attachment #8758928 - Attachment is obsolete: true
Attachment #8785613 - Flags: review?
Comment on attachment 8785613 [details] [diff] [review]
1095484-patch1.diff

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

This diff doesn't look like it's matching the changes in the bundle. I *think* it's a reverse diff, maybe? That is, I think you want:

hg diff -w -r d5239c42cbf1 -r tip

which produces a very different-looking diff?

::: js/lib/chatzilla-service.js
@@ +220,5 @@
> +
> +IRCProtocolHandler.prototype.allowPort =
> +function ircph_allowPort(port, scheme)
> +{
> +    return false;

We're changing this in another bug, right?
Attachment #8785613 - Flags: review?
Flags: needinfo?(gijskruitbosch+bugs)
Blake, see billm's comments in bug 1296885. What about turning this inside out and just allowing E10SUtils.jsm to query a protocol handler to find out if it must run in the parent? Since addons will have to do e10s work anyway, they can just opt-in to it (the one thing they can't do for themselves) and there would be much fewer changes to the base -- presumably just a few lines to E10SUtils.
Flags: needinfo?(mrbkap)
(In reply to Cameron Kaiser [:spectre] from comment #43)
> Blake, see billm's comments in bug 1296885. What about turning this inside
> out and just allowing E10SUtils.jsm to query a protocol handler to find out
> if it must run in the parent? Since addons will have to do e10s work anyway,

At the risk of adding yet another redirect, we should probably take this particular discussion to bug 1277410. I stopped pushing on the patch in that bug because all of the Necko folks I talked to were saying that we wanted to stop supporting extension-implemented nsIProtocolHandler altogether and I didn't want to add an API for something that was going away. I don't object at all to reviving that patch.
Flags: needinfo?(mrbkap)
This is a reversed version of attachment #8785613 [details] [diff] [review], which was generated by Mercurial Workbench, which I stupidly assumed would be a sensible bit of software and always diff from old->new without checking. Sorry!
Attachment #8785613 - Attachment is obsolete: true
Attachment #8808778 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8808778 [details] [diff] [review]
1095484-patch1b.diff

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

Conflict in jar.mn now that the drag and drop changes landed with nsDragAndDrop - do whatever we did in bug 1291500, I guess.

This looks pretty good to me.

::: js/lib/chatzilla-service.js
@@ +251,5 @@
> +        {
> +            const rdfSvc = Cc[RDFS_CONTRACTID].getService(Ci.nsIRDFService);
> +            const rdfDS = rdfSvc.GetDataSource("rdf:chrome");
> +            const resSelf = rdfSvc.GetResource("urn:mozilla:package:chatzilla");
> +            const resDisabled = rdfSvc.GetResource("http://www.mozilla.org/rdf/chrome#disabled");

We can probably remove this stuff in a separate bug.

::: js/lib/protocol-handlers.jsm
@@ +66,5 @@
> +    protocolFlags: protocolFlags,
> +
> +    allowPort(port, scheme)
> +    {
> +        return false;

make sure we merge in bug 1264774 in here.
Attachment #8808778 - Flags: review?(gijskruitbosch+bugs) → review+
http://hg.mozilla.org/chatzilla/rev/e30a625e63e6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Depends on: 1317340
Depends on: 1324217
You need to log in before you can comment on or make changes to this bug.