Closed Bug 807606 Opened 7 years ago Closed 7 years ago

Installing a Root Certificate crashes Firefox

Categories

(Firefox for Android :: General, defect, critical)

ARM
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 21

People

(Reporter: bugs, Unassigned)

References

()

Details

(Keywords: crash, reproducible, Whiteboard: [native-crash])

Attachments

(3 files, 6 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121024073032

Steps to reproduce:

If i try to install a Root Certificate eg. from http://www.cacert.org/index.php?id=3, firefox crashes inmediately.


Actual results:

ff crashes


Expected results:

normally, the root-ca-installation-window appears..
OS: Windows XP → Android
Can you provide the crash ID from the about:crashes page?
Severity: normal → critical
Keywords: crash
Hardware: x86 → ARM
Whiteboard: [native-crash]
E/BufferQueue(  128): [SurfaceView] connect: already connected (cur=1, req=1)
E/libEGL  ( 2010): EGLNativeWindowType 0x5c3b92d8 already connected to another API
E/libEGL  ( 2010): eglCreateWindowSurface:245 error 300b (EGL_BAD_NATIVE_WINDOW)
W/System.err( 2010): org.mozilla.gecko.gfx.GLController$GLControllerException: EGL window surface could not be created! Error 12299
W/System.err( 2010): 	at org.mozilla.gecko.gfx.GLController.provideEGLSurface(GLController.java:176)
W/System.err( 2010): 	at dalvik.system.NativeStart.run(Native Method)
W/System.err( 2010): 	at dalvik.system.NativeStart.run(Native Method)
I/Gecko   ( 2010): ###!!! ABORT: We need a context on Android: file ../../../gfx/layers/opengl/LayerManagerOGL.cpp, line 405
I/WindowState(  307): WIN DEATH: Window{41d83f40 org.mozilla.fennec/org.mozilla.fennec.App paused=false}
I/ActivityManager(  307): Process org.mozilla.fennec (pid 2010) has died.

bp-f2c75655-4e42-41a3-a5fd-00dbc2121101

I think this is a dupe of that malformed URI on Fennec launch bug.
Bug 763813.

Kats, can you confirm?
It's a dupe in that it's the same crash, but may require a different fix. There are multiple code paths that filter down into that crash, unfortunately :(
Status: UNCONFIRMED → NEW
Crash Signature: [@ mozalloc_abort]
Ever confirmed: true
Keywords: reproducible
Duplicate of this bug: 810100
Crash Signature: [@ mozalloc_abort] → [@ mozalloc_abort | system@framework@core.jar@classes.dex@0x7a11c]
Crash Signature: [@ mozalloc_abort | system@framework@core.jar@classes.dex@0x7a11c] → [@ mozalloc_abort] [@ mozalloc_abort | system@framework@core.jar@classes.dex@0x7a11c]
There's two more crash reports for this on bug 810100 (just marked as a duplicate of this) if they're helpful at all.
Duplicate of this bug: 824031
As I posted at https://bugzilla.mozilla.org/show_bug.cgi?id=801850#c3, this no longer crashes as of bug 785597. However trying to install a root cert still doesn't work because the window that pops up can't be interacted with.
I think the fix here is basically to provide an android implementation of nsICertificateDialogs which shows those dialogs as doorhangers or whatever instead of XUL windows.

This specific issue comes from the nsNSSCertificateDB::handleCACertDownload method calling nsNSSDialogs::ConfirmDownloadCACert via the nsICertificateDialogs interface, which goes on to open a new XUL window.
Crash Signature: [@ mozalloc_abort] [@ mozalloc_abort | system@framework@core.jar@classes.dex@0x7a11c] → [@ mozalloc_abort] [@ mozalloc_abort | system@framework@core.jar@classes.dex@0x7a11c] [@ mozalloc_abort(char const*) | NS_DebugBreak_P | mozilla::layers::LayerManagerOGL::Initialize(nsRefPtr<mozilla::gl::GLContext>, bool)]
Depends on: 785597
Attached patch Skeleton patch (obsolete) — Splinter Review
This is a skeleton of what the patch should be like, I think. Just need to properly implement the stubs in NSSDialogService.js to match the XUL prompts shown in http://mxr.mozilla.org/mozilla-central/source/security/manager/pki/src/nsNSSDialogs.cpp.
Attached patch WIP (obsolete) — Splinter Review
Slightly updated to implement a couple of the functions. The other function seem to require complex UI elements like password strength meters and tab panels, and I don't know if we can/want to stuff them into our little modal dialog popups. I'm not sure how to move forward here.
Attachment #701218 - Attachment is obsolete: true
Attached patch Updated WIP (obsolete) — Splinter Review
Updated WIP
Attachment #702381 - Attachment is obsolete: true
Comment on attachment 702836 [details] [diff] [review]
Updated WIP

Easy to remove the hack:
* rename l10n -> getString
* move the raw strings into a pippki.properties file in mobile
* Use Services.strings.createBundle to load the properties file and fetch the string
Attached patch Implement NSS dialogs in fennec (obsolete) — Splinter Review
Ian, I'd like to get your feedback on the dialogs I implemented for this. There are screenshots at http://people.mozilla.com/~kgupta/bug/807606/

Here are steps on how to cause these dialogs to display:

For the "confirm download" dialog:
- Click on this link: https://www.mozilla.org/certs/mozilla-root.crt
For the "view cert" dialog:
- Same as above, but then click on the "View" button
For the "notify exists" dialog:
- If you do the steps to get to the "confirm download" dialog, and then install the cert and click on the link again, you should see this.

There are three other dialogs in the API but as far as I can tell we can never invoke those on Fennec so I didn't bother implementing those.
Attachment #702836 - Attachment is obsolete: true
Attachment #704673 - Flags: review?(mark.finkle)
Attachment #704673 - Flags: feedback?(ibarlow)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> Here are steps on how to cause these dialogs to display:

This is on desktop. The same instructions work on Fennec, but only with my patch applied. There is a try build going at https://tbpl.mozilla.org/?tree=Try&rev=102322887476 with the patch.
Comment on attachment 704673 [details] [diff] [review]
Implement NSS dialogs in fennec

>diff --git a/mobile/android/components/NSSDialogService.js b/mobile/android/components/NSSDialogService.js
>+      if (response.trustSSL == "true") aTrust.value |= Ci.nsIX509CertDB.TRUSTED_SSL;
>+      if (response.trustEmail == "true") aTrust.value |= Ci.nsIX509CertDB.TRUSTED_EMAIL;
>+      if (response.trustSign == "true") aTrust.value |= Ci.nsIX509CertDB.TRUSTED_OBJSIGN;

I can live with this format :)

>+  certInfo: function(aLabelKey, aValue) {
>+    return this.getString(aLabelKey) + ": " + aValue;

I guess this will be OK with l10n

>+  viewCert: function(aCtx, aCert) {
>+    this.showPrompt(this.getString("certmgr.title"),
>+                    this.getString("certmgr.subjectinfo.label") + "\n\n"
>+                    + this.certInfo("certmgr.certdetail.cn", aCert.commonName) + "\n"
>+                    + this.certInfo("certmgr.certdetail.o", aCert.orgnization) + "\n"
>+                    + this.certInfo("certmgr.certdetail.ou", aCert.organizationalUnit) + "\n"
>+                    + this.certInfo("certmgr.certdetail.serialnumber", aCert.serialNumber) + "\n\n" +
>+                    this.getString("certmgr.issuerinfo.label") + "\n\n"
>+                    + this.certInfo("certmgr.certdetail.cn", aCert.issuerCommonName) + "\n"
>+                    + this.certInfo("certmgr.certdetail.o", aCert.issuerOrgnization) + "\n"
>+                    + this.certInfo("certmgr.certdetail.ou", aCert.issuerOrganizationUnit) + "\n\n" +
>+                    this.getString("certmgr.validity.label") + "\n\n"
>+                    + this.certInfo("certmgr.issued", aCert.validity.notBeforeLocalDay) + "\n"
>+                    + this.certInfo("certmgr.expires", aCert.validity.notAfterLocalDay) + "\n\n" +
>+                    this.getString("certmgr.fingerprints.label") + "\n\n"
>+                    + this.certInfo("certmgr.certdetail.sha1fingerprint", aCert.sha1Fingerprint) + "\n"
>+                    + this.certInfo("certmgr.certdetail.md5fingerprint", aCert.md5Fingerprint),
>+                    [ this.getString("nssdialogs.ok.label") ],
>+                    []);

O_o but not much else we can do without a custom dialog. Given how infrequently this might be used, it would probably be fine to start.

>diff --git a/mobile/android/locales/en-US/chrome/pippki.properties b/mobile/android/locales/en-US/chrome/pippki.properties

>+downloadCert.trustSSL=Trust this CA to identify websites.
>+downloadCert.trustEmail=Trust this CA to identify email users.
>+downloadCert.trustObjSign=Trust this CA to identify software developers.

For mobile space issues, I'd like to drop the "this CA" from the above 3 strings
Attachment #704673 - Flags: review?(mark.finkle) → review+
Hi Kats, this looks good. Couple comments:

1. Move the checkboxes over to the right, like we do in Clear Private Data http://cl.ly/image/2R2I2L0V3D20

2. Tweak the certificate styling a little so that it reads better: http://cl.ly/image/1t1u0j3E3U19
This is an update to use the new label view I added in part 2. I also updated the checkbox strings as requested.
Attachment #704673 - Attachment is obsolete: true
Attachment #704673 - Flags: feedback?(ibarlow)
Attachment #705575 - Flags: review?(mark.finkle)
(In reply to Ian Barlow (:ibarlow) from comment #18)
> Hi Kats, this looks good. Couple comments:
> 
> 1. Move the checkboxes over to the right, like we do in Clear Private Data
> http://cl.ly/image/2R2I2L0V3D20

After fighting with Android for half the day I admitted defeat in trying to do this.

> 2. Tweak the certificate styling a little so that it reads better:
> http://cl.ly/image/1t1u0j3E3U19

The updated patches do this.
Attachment #705571 - Flags: review?(wjohnston) → review+
Comment on attachment 705572 [details] [diff] [review]
Part 2 - Add label input types

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

Want your feedback on the HTML idea.

::: mobile/android/base/PromptService.java
@@ +207,5 @@
>                  mView = (View)spinner;
> +            } else if (mType.equals("label")) {
> +                // not really an input, but a way to add labels and such to the dialog
> +                TextView view = new TextView(GeckoApp.mAppContext);
> +                view.setText(mLabel);

I think a neat way to do this would be to use Html.fromHtml(String) which should give us a spannable that can be formatted whatever nutso way you want... Have you tried? I think I like that, but I'd be willing to listen if someone is vehemently against it....
Attachment #705572 - Flags: review?(wjohnston)
Scoobidiver, this is about a crash with specific steps (the ones from bug 824031 comment #0 are pretty explicit and easy to follow) that happens basically because we are missing this feature.

That said, we actually seem to not crash any more from what I see in Nightly on my ASUS TF101 tablet, it looks like we are now actually displaying the XUL document (or "window"/dialog) and that makes the browser unusable as nothing can be clicked there but it occupies the whole content/Gecko area, no matter what you are trying to do or load.
Crash Signature: [@ mozalloc_abort] [@ mozalloc_abort | system@framework@core.jar@classes.dex@0x7a11c] [@ mozalloc_abort(char const*) | NS_DebugBreak_P | mozilla::layers::LayerManagerOGL::Initialize(nsRefPtr<mozilla::gl::GLContext>, bool)]
(In reply to Wesley Johnston (:wesj) from comment #23)
> I think a neat way to do this would be to use Html.fromHtml(String) which
> should give us a spannable that can be formatted whatever nutso way you
> want... Have you tried? I think I like that, but I'd be willing to listen if
> someone is vehemently against it....

Oh, I didn't see that class. I'll give it a try and see how well it works.

(In reply to Scoobidiver from comment #24)
> Is this bug about a crash (there are still ones after bug 785597 was fixed -
> see
> https://crash-stats.mozilla.com/report/
> list?signature=mozalloc_abort%28char%20const*%29%20|%20NS_DebugBreak_P%20|%20
> mozilla%3A%3Alayers%3A%3ALayerManagerOGL%3A%3AInitialize%28nsRefPtr%3Cmozilla
> %3A%3Agl%3A%3AGLContext%3E%2C%20bool%29), a new feature, or both?

That particular crash signature has a few different causes. Bug 785597 fixed one of them, but there are still others that have not yet been fixed. I will file a new bug for the remaining crashes.

(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #25)
> That said, we actually seem to not crash any more from what I see in Nightly
> on my ASUS TF101 tablet, it looks like we are now actually displaying the
> XUL document (or "window"/dialog) and that makes the browser unusable as
> nothing can be clicked there but it occupies the whole content/Gecko area,
> no matter what you are trying to do or load.

Correct, and the patches on the bug aim to correct that behaviour.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #26)
> That particular crash signature has a few different causes. Bug 785597 fixed
> one of them, but there are still others that have not yet been fixed. I will
> file a new bug for the remaining crashes.
Done. Filed bug 834243.
Comment on attachment 705575 [details] [diff] [review]
Part 3 - Implement NSS dialogs in Fennec

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

These strings are all duplicated from:

http://mxr.mozilla.org/mozilla-central/source/security/manager/locales/en-US/chrome/pippki/pippki.dtd

but I can't find a "good" way to pull them from there. DOMParser will do it, but that's sorta a hack. Hopefully mfinkle knows something smarter we can do.
Now with HTML goodness
Attachment #705572 - Attachment is obsolete: true
Attachment #705901 - Flags: review?(wjohnston)
Updated to use HTML
Attachment #705575 - Attachment is obsolete: true
Attachment #705575 - Flags: review?(mark.finkle)
Attachment #705903 - Flags: review?(mark.finkle)
(In reply to Wesley Johnston (:wesj) from comment #28)
> Comment on attachment 705575 [details] [diff] [review]
> Part 3 - Implement NSS dialogs in Fennec
> 
> Review of attachment 705575 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> These strings are all duplicated from:
> 
> http://mxr.mozilla.org/mozilla-central/source/security/manager/locales/en-US/
> chrome/pippki/pippki.dtd
> 
> but I can't find a "good" way to pull them from there. DOMParser will do it,
> but that's sorta a hack. Hopefully mfinkle knows something smarter we can do.

With our desire to kill all toolkit strings, I like forking the files to like in /mobile
Attachment #705901 - Flags: review?(wjohnston) → review+
Comment on attachment 705903 [details] [diff] [review]
Part 3 - Implement NSS dialogs in Fennec (v3)

This HTML stuff is blowing my mind, but it's cool.
Attachment #705903 - Flags: review?(mark.finkle) → review+
downloadCert.trustEmail=Trust to identify email users.

Can someone explain to me this string? I can understand how a certificate can be used to trust websites, software packages or mail servers, but honestly not "email users".
I'm not entirely sure, to be honest. I copied the string over from the desktop version at http://mxr.mozilla.org/mozilla-central/source/security/manager/locales/en-US/chrome/pippki/pippki.dtd#41

I assume that this is related to PGP or similar signed emails but I could be wrong. It may not make sense on mobile at all.
Duplicate of this bug: 848737
Duplicate of this bug: 855063
Depends on: 1281661
You need to log in before you can comment on or make changes to this bug.