989875 - [e10s] "Untrusted Connection" page's "Add Exception" button does nothing

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Chris Peterson [:cpeterson]]

STR:
1. Open new e10s window
2. Load https://svn.resiprocate.org/rep/resiprocate/
3. See the "Untrusted Connection" page
4. Click "I Understand the Risks" and "Add Exception" button

RESULT:
Nothing happens. Clicking the "Add Exception" button should open a new dialog to import the site certificate.

Comment 1

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

My understanding is that when a child process is notified of a load failure, the docshell loads an error page in the child process. The certificate override page should be loaded in the parent process instead, to allow for privileged actions to occur.

Comment 5

[Jim Mathies [:jimm]]

Attachment: wip v.1

This fixes the cert and malware pages. I need to check a couple more to see what else might be broken.

Comment 6

[Jim Mathies [:jimm]]

Attachment: patch v.1

This also fixes the networking error page, which generally worked, but the networking call we make in browser.js wasn't getting invoked.

I'm thinking that ClickEventHandler might do well as a jsm if I can keep it all working right with the change in scope. Not sure it's worth it though.

Comment 7

[Carl Ponder]

I've been running into an additional certificate problem, where I need to authenticate to the ID cponder@.... instead of just "cponder" which I had been using previously. When I click on other page links they are not working, I believe they are looking for a "cponder" certificate but inheriting a "cponder@...." certificate instead. The linked pages are not asking for any authentication, however. They just quit working since my last set of Ubuntu updates a week or so ago.

This is a problem for me with both FireFox and Opera but not Chrome. The underlying problem may be the authentication server I'm stuck with, but these certificate idiosyncracies with FireFox are complicating the debug.

Does this have anything to do with SSH updates since the HeartBleed bug was announced? Are all the client- and server- pieces working together properly?

Comment 8

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Carl, that sounds like a different bug. Please file a new bug under Core :: Security: PSM with steps to reproduce the problem. Thanks!

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8439448 [details] [diff] [review]
patch v.1

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

Awesome, looks great

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 8439448 [details] [diff] [review]
patch v.1

Hey matt, mind doing an e10s review? This patch is similar to our handling in native fennec and metro for certain about pages that load in the child.

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8439448 [details] [diff] [review]
patch v.1

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

Looks good to me.  I'm not a peer for this code, but you can check it in if Felipe is cool with that.  Some very minor stylistic suggestions below.

::: browser/base/content/browser.js
@@ +2193,5 @@
> +      break;
> +    }
> +  },
> +
> +  onAboutCertError: function BrowserOnClick_onAboutCertError(aTarget, aElementId, aIsTopFrame, aLocation) {

Maybe rename aTarget to aBrowser just to be more specific?

@@ +2244,5 @@
>  
>      }
>    },
>  
> +  onAboutBlocked: function BrowserOnClick_onAboutBlocked(aTarget, aElementId, aIsMalware, aIsTopFrame, aLocation) {

It looks like aTarget is unused. Remove it?

::: browser/base/content/content.js
@@ +258,5 @@
>        .getService(Ci.nsIEventListenerService)
>        .addSystemEventListener(global, "click", this, true);
>    },
>  
> +  handleEvent: function(aEvent) {

Most (but not all) of content.js is using non-prefixed parameter names. Maybe we should have an executive decision on which standard to converge to?

@@ +310,5 @@
> +
> +  onAboutCertError: function (aTargetElm, aOwnerDoc) {
> +    let json = {};
> +    json.location = aOwnerDoc.location.href;
> +    json.elementId = aTargetElm.getAttribute("id");

You could use object literals here if you find it clearer (I do, but YMMV):

  let json = {
    location: aOwnerDoc.location.href,
    elementId: aTargetElm.getAttribute("id"),
    ...
  };

Comment 12

[Jim Mathies [:jimm]]

Thanks for the review! All comments addresses, will push a final try test run then land.

(In reply to Matt Brubeck (:mbrubeck) from comment #11)
> Comment on attachment 8439448 [details] [diff] [review]
> patch v.1
> 
> Review of attachment 8439448 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: browser/base/content/content.js
> @@ +258,5 @@
> >        .getService(Ci.nsIEventListenerService)
> >        .addSystemEventListener(global, "click", this, true);
> >    },
> >  
> > +  handleEvent: function(aEvent) {
> 
> Most (but not all) of content.js is using non-prefixed parameter names.
> Maybe we should have an executive decision on which standard to converge to?

Maybe just the result of slack reviews? I'm not aware of a global decision that's been made that we can skip doing this in js.

I'll ask around, easy enough to clean up either way in a follow up.

Comment 13

[Jim Mathies [:jimm]]

 https://hg.mozilla.org/integration/fx-team/rev/9879665a17bb

Comment 14

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #13)
>  https://hg.mozilla.org/integration/fx-team/rev/9879665a17bb

I made one last minute change here for a failing test. Instead of sending the Services.io.offline reset async, I send it sync. This was caught by a failing test in my last try push - 

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug435325.js#59

Comment 15

[Ed Morley [:emorley]]

Backed out for leaks:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42866199&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=42866038&tree=Fx-Team
TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug409481.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/web-panels.xul]

Did the try run pick this up? (I can't see a link in the bug)

remote:   https://hg.mozilla.org/integration/fx-team/rev/bd66f521c2b8

Comment 16

[Jim Mathies [:jimm]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #15)
> Backed out for leaks:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=42866199&tree=Fx-Team
> https://tbpl.mozilla.org/php/getParsedLog.php?id=42866038&tree=Fx-Team
> TEST-UNEXPECTED-FAIL |
> browser/base/content/test/general/browser_bug409481.js | leaked 1 window(s)
> until shutdown [url = chrome://browser/content/web-panels.xul]
> 
> Did the try run pick this up? (I can't see a link in the bug)
> 
> remote:   https://hg.mozilla.org/integration/fx-team/rev/bd66f521c2b8

Sorry about that. Seems to have been introduced between my try push yesterday and the fresh pull I did this morning.

Comment 17

[Jim Mathies [:jimm]]

Attachment: patch v.2

This was in my patch, I left a few mm listeners registered. The only changes here are:

1) BrowserOnClick.init is now called in onLoad rather than globally.
2) I've added an uninit which gets called in onUnload.

https://tbpl.mozilla.org/?tree=Try&rev=83b8cddfb1b1

Sorry thought this went through try, my mistake.

Comment 18

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8449625 [details] [diff] [review]
patch v.2

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

::: browser/base/content/browser.js
@@ +2295,5 @@
> +    let mm = window.messageManager;
> +    mm.addMessageListener("Browser:CertExceptionError", this);
> +    mm.addMessageListener("Browser:SiteBlockedError", this);
> +    mm.addMessageListener("Browser:NetworkError", this);
> +    dump("*************** init\n");

Remove the dump() call before landing.

@@ +2303,5 @@
> +    let mm = window.messageManager;
> +    mm.removeMessageListener("Browser:CertExceptionError", this);
> +    mm.removeMessageListener("Browser:SiteBlockedError", this);
> +    mm.removeMessageListener("Browser:NetworkError", this);
> +    dump("*************** uninit\n");

Ditto.

Comment 19

[Jim Mathies [:jimm]]

Thanks!

https://hg.mozilla.org/integration/fx-team/rev/b5847adafb3e

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b5847adafb3e

Comment 21

[CAK]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0

Today's build caused the "Add Exception" button not to work again.

Comment 22

[Carl Ponder]

With Ubuntu FireFox 30.0 I'm finding that the "Add Permenant Exception" is not working but the "Add Exception" button does work. I assume this has more to do with an incompatibility of the security certifcates rather than a new bug in FireFox itself.

Comment 23

[Jan Melcher]

This bug is still present in nightly 34.0a1 on Ubuntu 14.04.

Comment 24

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Yogu - generally when bugs re-appear after being fixed, we file a new bug to track them (because in many cases the root cause is different). Please file a new bug with steps to reproduce the issue.

Comment 25

[Jan Melcher]

Ok, created bug 1053456.