Closed Bug 989875 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Security, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 33
Tracking Status
e10s + ---
firefox31 --- affected

People

(Reporter: cpeterson, Assigned: jimm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
tracking-e10s: --- → +
Assignee: nobody → ally
Duplicate of this bug: 1014675
Assignee: ally → jmathies
Status: NEW → ASSIGNED
Attached patch wip v.1 (obsolete) — Splinter Review
This fixes the cert and malware pages. I need to check a couple more to see what else might be broken.
Attached patch patch v.1 (obsolete) — Splinter Review
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.
Attachment #8439427 - Attachment is obsolete: true
Attachment #8439448 - Flags: feedback?(felipc)
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?
Carl, that sounds like a different bug. Please file a new bug under Core :: Security: PSM with steps to reproduce the problem. Thanks!
Comment on attachment 8439448 [details] [diff] [review]
patch v.1

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

Awesome, looks great
Attachment #8439448 - Flags: feedback?(felipc) → feedback+
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.
Attachment #8439448 - Flags: review?(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"),
    ...
  };
Attachment #8439448 - Flags: review?(mbrubeck) → review+
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.
(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
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
(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.
Attached patch patch v.2Splinter Review
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.
Attachment #8439448 - Attachment is obsolete: true
Attachment #8449625 - Flags: review?(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.
Attachment #8449625 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/mozilla-central/rev/b5847adafb3e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Depends on: 1034581
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.
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.
This bug is still present in nightly 34.0a1 on Ubuntu 14.04.
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.
Ok, created bug 1053456.
QA Whiteboard: [good first verify]
Depends on: 1106075
You need to log in before you can comment on or make changes to this bug.