940206 - nsIWebContentHandlerRegistrar::registerProtocolHandler doesn't work in e10s

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

either that, or just the test is broken, but when run in e10s mode:

2:27.50 TEST-START | chrome://mochitests/content/browser/browser/base/content/test/general/browser_registerProtocolHandler_notification.js
 2:27.52 *** registerProtocolHandler(testprotocol,https://example.com/foobar?uri=%s,Test Protocol)
 2:27.52 ************************************************************
 2:27.52 * Call to xpconnect wrapped JSObject produced this error:  *
 2:27.52 [Exception... "'[JavaScript Error: "Permission denied for <http://example.com> to create wrapper for object of class UnnamedClass" {file: "resource://gre/modules/PrivateBrowsingUtils.jsm" line: 25}]' when calling method: [nsIWebContentHandlerRegistrar::registerProtocolHandler]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: http://example.com/browser/browser/base/content/test/general/browser_registerProtocolHandler_notification.html :: <TOP_LEVEL> :: line 12"  data: yes]
 2:27.52 ************************************************************
 2:27.52 JavaScript error: http://example.com/browser/browser/base/content/test/general/browser_registerProtocolHandler_notification.html, line 12: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "Permission denied for <http://example.com> to create wrapper for object of class UnnamedClass" {file: "resource://gre/modules/PrivateBrowsingUtils.jsm" line: 25}]' when calling method: [nsIWebContentHandlerRegistrar::registerProtocolHandler]
 2:27.53 TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/general/browser_registerProtocolHandler_notification.js | Console message: [JavaScript Error: "Permission denied for <http://example.com> to create wrapper for object of class UnnamedClass" {file: "resource://gre/modules/PrivateBrowsingUtils.jsm" line: 25}]
 2:27.53 TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/general/browser_registerProtocolHandler_notification.js | Console message: [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "Permission denied for <http://example.com> to create wrapper for object of class UnnamedClass" {file: "resource://gre/modules/PrivateBrowsingUtils.jsm" line: 25}]' when calling method: [nsIWebContentHandlerRegistrar::registerProtocolHandler]" {file: "http://example.com/browser/browser/base/content/test/general/browser_registerProtocolHandler_notification.html" line: 12}]
 2:30.66 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_registerProtocolHandler_notification.js | 100
 2:30.66 Stack trace:
 2:30.66     JS frame :: chrome://mochitests/content/browser/browser/base/content/test/general/head.js :: <TOP_LEVEL> :: line 67
 2:30.66     native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
 2:30.66 
 2:30.66 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_registerProtocolHandler_notification.js | Notification box should be displayed
 2:30.66 Stack trace:
 2:30.66     JS frame :: chrome://mochitests/content/browser/browser/base/content/test/general/browser_registerProtocolHandler_notification.js :: test/< :: line 21
 2:30.66     JS frame :: chrome://mochitests/content/browser/browser/base/content/test/general/head.js :: <TOP_LEVEL> :: line 82
 2:30.66     JS frame :: chrome://mochitests/content/browser/browser/base/content/test/general/head.js :: <TOP_LEVEL> :: line 68
 2:30.66     native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
 2:30.66 
 2:30.66 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_registerProtocolHandler_notification.js | uncaught exception - TypeError: notification is null at chrome://mochitests/content/browser/browser/base/content/test/general/browser_registerProtocolHandler_notification.js:22
 2:30.66 Stack trace:
 2:30.66     JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 1166
 2:30.66     native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
 2:30.66 
 2:30.66 JavaScript error: chrome://mochitests/content/browser/browser/base/content/test/general/browser_registerProtocolHandler_notification.js, line 22: notification is null

Comment 1

[(not currently active) Ted Mielczarek]

I get the same error trying to use it in the web console, so I think it's legit broken:

navigator.registerProtocolHandler("mailto","https://www.fastmail.com/action/compose/?mailto=%s","FastMail");

[Exception... "[JavaScript Error: "aWindow.QueryInterface is not a function" {file: "resource://gre/modules/PrivateBrowsingUtils.jsm" line: 49}]'[JavaScript Error: "aWindow.QueryInterface is not a function" {file: "resource://gre/modules/PrivateBrowsingUtils.jsm" line: 49}]' when calling method: [nsIWebContentHandlerRegistrar::registerProtocolHandler]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1"  data: yes]

Comment 2

[Ian Nartowicz]

I have a custom protocol handler (for magnet: scheme) in an addon and it apparently stops working with e10s, although I see no errors in the console.  Works fine in nightly with autostart.1 pref set to false, but just stops handling magnet links when e10s is enabled.

Comment 3

[Brian Fernandes]

Would like to add FireBible to the list of affected extensions. The sword:// & bible:// protocols that FireBible adds no longer work with e10s enabled, pretty much breaking the extension entirely. http://thegoan.com/firebible

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Use the Services jsm to simplify a few lines.

There were a few places that could be simplified by using Services.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: wip for registerProtocolHandler

This should make registerProtocolHandler work, I still need to test it in the e10s case. Unfortunately it appears that test_registerHandler.html requires registerContentHandler to work as well. I have a plan for making that work, dealing with pref branch observers that I'll base on top of this patch.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 7

[Ian Nartowicz]

Does this bug, or will this fix, also cover registerProtocolHandler with nsIProtocolHandler?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Ian, I don't think this bug will affect that. What API, exactly are you talking about?

Comment 9

[Ian Nartowicz]

I think you're right.  I've finally got a custom scheme working in my addon using the component registrar in a frame script.  It doesn't work from a javascript component any more.  It wasn't working in a frame script, now it is.  I don't know what I was doing wrong.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Small patch to the test

This test passes the parameters to is() backwards, making it confusing to debug. I corrected that. While I was here, I fixed a todo() that wasn't written correctly either, that became a todo_is, like the rest of them.

Dao, you're the last person to review anything in this file, please let me know if there's a better reviewer for these patches.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix registerProtocolHandler

I'm playing a little fast and loose with the URI stuff, but I think it's correct.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix registerContentHandler

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix an odd bug

According to the comment I'm removing, this isn't supposed to throw, but the function returns undefined in this case and nobody checks for it, so we'd throw anyway. Furthermore, according to the current spec[1], we are supposed to throw.

[1] https://html.spec.whatwg.org/multipage/webappapis.html#dom-navigator-registerprotocolhandler

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Enable this test on e10s.

With these patches, this test now passes.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

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

With these patches, registerContentHandler should work, but I still need to implement nsIWebContentConverterService for the feed page to work properly (the current implementation will try to set prefs from the content process. It should be possible to do this with relatively little work and some refactoring (plus some pref observers). Should I do that in this bug?

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8641355 [details] [diff] [review]
Fix registerProtocolHandler

>+const Utils = {
>+  _makeURI(aURL, aOriginCharset, aBaseURI) {
>+    var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+                              .getService(Components.interfaces.nsIIOService);
>+    return ioService.newURI(aURL, aOriginCharset, aBaseURI);

You can use Services.io here.

You can also get rid of the underscore prefixes for these method names since Utils isn't exported from this script.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8641356 [details] [diff] [review]
Fix registerContentHandler

>     if (aWindowOrBrowser) {
>-      var uri = Utils._checkAndGetURI(aURIString, aWindowOrBrowser);
>+      var haveWindow = (aWindowOrBrowser instanceof Ci.nsIDOMWindow);
>+      var uri;
>+      var notificationBox;
>+      if (haveWindow) {
>+        uri = Utils._checkAndGetURI(aURIString, aWindowOrBrowser);
>+
>+        var browserWindow = this._getBrowserWindowForContentWindow(aWindowOrBrowser);
>+        var browserElement = this._getBrowserForContentWindow(browserWindow, aWindowOrBrowser);
>+        notificationBox = browserWindow.gBrowser.getNotificationBox(browserElement);

You could use browserElement.getTabBrowser().getNotificationBox(browserElement) here like you did elsewhere.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With comments addressed

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d754f7e1cb9

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9d754f7e1cb9

Comment 21

[Brian Fernandes]

Using the nightly build: 42.0a1 (2015-08-04) - my protocol handler is still defunct when e10s is enabled. Is this issue supposed to be fixed in the above build, or have I misunderstood the extent of the fix?

I see a "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." error. Can't see any protocol related errors in the log though.

When e10s is disabled, the protocol handler works just fine.

Extension can be installed from: http://thegoan.com/firebible for testing - the extension signing check will need to be disabled.

Comment 22

[Ian Nartowicz]

A quick look at your code suggests that FireBible is not using a web content protocol handler.  It is using a chrome custom protocol handler (nsIProtocolHandler).  This is the same confusion I had.  This bug is not relevant. Look at the latest beta versions of my addon for code that works (frame.js is the frame script, frame.jsm contains the handler):
https://addons.mozilla.org/en-us/firefox/addon/torrent-status-tool/

To the best of my knowledge, the only way to use these with e10s enabled is to manually register your protocol handler in a frame script.  I could not find as way to do this using the old chrome.manifest registration.  Protocol handlers registered in a component exist but are never called to handle content.

Comment 23

[Brian Fernandes]

Ian, appreciate the explanation, thank you. It does look like the the nsIProtocolHandler issue is being addressed in bug #1176646 though, so I can wait on a fix there before testing further; hopefully changes to the extension will not be required.