807606 - Installing a Root Certificate crashes Firefox

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Stefan]

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..

Comment 1

[Scoobidiver (away)]

Can you provide the crash ID from the about:crashes page?

Comment 2

[Aaron Train [:aaronmt]]

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.

Comment 3

[Aaron Train [:aaronmt]]

Bug 763813.

Kats, can you confirm?

Comment 4

[Kartikaya Gupta (email:[e-mail])]

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 :(

Comment 6

[Dave Miller [:justdave]]

There's two more crash reports for this on bug 810100 (just marked as a duplicate of this) if they're helpful at all.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Skeleton patch

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.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

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.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Updated WIP

Updated WIP

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Implement NSS dialogs in fennec

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.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 17

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 18

[Ian Barlow (:ibarlow)]

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

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Add padding/scrollability to inputs

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Add label input types

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Implement NSS dialogs in Fennec

This is an update to use the new label view I added in part 2. I also updated the checkbox strings as requested.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 23

[Wesley Johnston (:wesj)]

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....

Comment 24

[Scoobidiver (away)]

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|%20mozilla%3A%3Alayers%3A%3ALayerManagerOGL%3A%3AInitialize%28nsRefPtr%3Cmozilla%3A%3Agl%3A%3AGLContext%3E%2C%20bool%29), a new feature, or both?

Comment 25

[Robert Kaiser]

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.

Comment 26

[Kartikaya Gupta (email:[e-mail])]

(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 (:[e-mail]) 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.

Comment 27

[Scoobidiver (away)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 28

[Wesley Johnston (:wesj)]

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.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Add label input type (v2)

Now with HTML goodness

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Implement NSS dialogs in Fennec (v3)

Updated to use HTML

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 32

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 33

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7773a5b156
https://hg.mozilla.org/integration/mozilla-inbound/rev/c87c74dc9169
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a7bc20e2aa

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e7773a5b156
https://hg.mozilla.org/mozilla-central/rev/c87c74dc9169
https://hg.mozilla.org/mozilla-central/rev/a8a7bc20e2aa

Comment 35

[Francesco Lodolo [:flod]]

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".

Comment 36

[Kartikaya Gupta (email:[e-mail])]

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.