1144600 - [Flame][Browser] nsNSSDialogHelper::openDialog() implementation for b2g

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Norry.L.F(Leave from Mozilla)]

Attachment: Crash.mp4

[1.Description]:
[Flame v2.2 & v3.0][Browser] Device will reboot and a crash notifacation pops up after user tap submit button to apply a cetificate via web page. 
Found time:17:03
Attachment:Crash.mp4 & logcat_1703.txt
Crash link: 
**Title
    B2G 37.0 Crash Report [@ mozilla::gl::GLContextProviderEGL::CreateForWindow(nsIWidget*) ]
**Crash Report
    https://crash-stats.mozilla.com/report/index/f5481f60-4f14-4dc1-a9ec-e22d22150317

[2.Testing Steps]:
Prerequisite:There is a Microsoft certificate server in LAN. 
1.Open browser.
2.Open certificate server site.
3.Tap "申请一个证书".
4.Tap "用户证书".
5.Tap "提交".

[3.Expected Result]: 
5.Device shouldn't reboot.

[4.Actual Result]: 
5.Device will reboot and a crash notification pops up.

[5.Reproduction build]: 
Flame 2.1 build:(unaffected)
Build ID               20150317161204
Gaia Revision          13c85d57f49b4bfd657ff674f2b530c141c94803
Gaia Date              2015-03-17 13:31:54
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/2fbd284621e2
Gecko Version          34.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150317.195338
Firmware Date          Tue Mar 17 19:53:47 EDT 2015
Bootloader             L1TC000118D0

Flame 2.2 build:(affected)
Build ID               20150317162504
Gaia Revision          306772a58335ac4cad285d27c3805090a8cc6886
Gaia Date              2015-03-17 17:12:36
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/83251e534b33
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150317.195036
Firmware Date          Tue Mar 17 19:50:45 EDT 2015
Bootloader             L1TC000118D0

Flame 3.0 build:(affected)
Build ID               20150317073344
Gaia Revision          738987bd80b0ddb4ccf853855388c2627e19dcc1
Gaia Date              2015-03-17 02:27:51
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/008b3f65a7e0
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150317.111450
Firmware Date          Tue Mar 17 11:15:00 EDT 2015
Bootloader             L1TC000118D0

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
Free Test

Comment 1

[Norry.L.F(Leave from Mozilla)]

Attachment: logcat_1703.txt

Comment 2

[Kelsey Gilbert [:jgilbert]]

:kamidphish and I are primarily on WebGL, not B2G layers.
:milan (currently PTO) or :jrmuizel would better be able to direct this.

Comment 3

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

Based on experience with Fennec I'm guessing this is because B2G doesn't provide an implementation for the "mozilla.org/nsCertificateDialogs;1" contract and so gecko tries to open a new XUL window which doesn't go over so well. Bug 807606 was the Fennec bug. I could be totally wrong, but if I'm right this isn't a gfx specific bug.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

What kats says is probably a good guess. However, it looks like the steps to reproduce in bug 807606 don't reproduce on the flame. It just downloads the file.

Comment 5

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

Is it possible to create a publicly accessible page to reproduce this problem?

Comment 6

[Norry.L.F(Leave from Mozilla)]

Hi Kartikaya,

Sorry for that we can't create a public page since this is for our internal server. If you want to ask this page, you can provide your IP to me, and we can give it a permission.

Thanks.

Comment 7

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

Ok, I emailed you my public IP, if you can add it I can try to reproduce the problem and see what's going on.

Comment 8

[Sotaro Ikeda [:sotaro]]

I checked attachment 8579254 [details] and comment 0. GLContextProviderEGL::CreateForWindow() was failed because, GLContextProviderEGL is going to use FramebufferSurface that is already used for CompositorOGL's OpenGL composition. The following log says that.

> 03-18 17:03:58.759 E/BufferQueue(  222): [FramebufferSurface] connect: already connected (cur=1, req=1)
> 03-18 17:03:58.759 E/libEGL  (  222): EGLNativeWindowType 0xb30bd808 already connected to another API
> 03-18 17:03:58.759 E/libEGL  (  222): eglCreateWindowSurface:414 error 3003 (EGL_BAD_ALLOC)

FYI: FramebufferSurface related diagram.

https://github.com/sotaroikeda/firefox-diagrams/blob/master/widget/widget_HwcComposer2D__FirefoxOS_1_4.pdf?raw=true

Comment 9

[Sotaro Ikeda [:sotaro]]

From the thread stacks dump, ContentParent::RecvKeygenProcessValue() triggered nsNSSDialogHelper::openDialog() and it caused new Widget and Compositor creation.

Comment 10

[Sotaro Ikeda [:sotaro]]

I confirmed this is a bug of <keygen> support on b2g. The following sample cause the same crash.

https://bug474958.bugzilla.mozilla.org/attachment.cgi?id=380749

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/keygen

Comment 11

[Sotaro Ikeda [:sotaro]]

nsNSSDialogs::SetPassword() or nsNSSDialogHelper::openDialog() might be necessary to be changed for b2g as not to create new Widget.

https://dxr.mozilla.org/mozilla-central/source/security/manager/pki/src/nsNSSDialogs.cpp?from=nsNSSDialogs::SetPassword%28#72
https://dxr.mozilla.org/mozilla-central/source/security/manager/pki/src/nsNSSDialogHelper.cpp#20


On b2g, dialog does not create a new Widget. It is done by gaia or like Bug 767818 and Bug 794680.

Comment 12

[Sotaro Ikeda [:sotaro]]

From comment 11, this bus is not a graphic bug, but is is security module's dialog implementation's bug.

Comment 13

[Sotaro Ikeda [:sotaro]]

Change to a correct module.

Comment 14

[Sotaro Ikeda [:sotaro]]

:bajaj, can't this bug have 2.2+ flag? As in comment 10, <keygen> tag could cause system reboot.

Comment 15

[Bobby Chien]

bhavana, could you help to find security owner?

Comment 16

[bhavana bajaj [:bajaj]]

Paul, can you help find an owner on this ? Also, I am not sure if we'd block the release on this given this is not a recent regression, any input here?

Comment 17

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to bhavana bajaj [:bajaj] from comment #16)
> Paul, can you help find an owner on this ? Also, I am not sure if we'd block
> the release on this given this is not a recent regression, any input here?

Why would say its not a recent regression? It seems to work on 2.1, but not 2.2? And more to the point, <keygen> is a standardized HTML element. Honestly I've never seen it used, but I can't see any reason why would wouldn't block on this.

Comment 18

[[:fabrice] Fabrice Desré]

Sotaro is correct in comment 11. We need to proxy the dialog creation and management to gaia's system app. It seems that we have 8 different dialogs implemented in XUL, but I don't know if we need them all to implement <keygen> properly:

chrome://pippki/content/changepassword.xul
chrome://pippki/content/downloadcert.xul
chrome://pippki/content/clientauthask.xul
chrome://pippki/content/certpicker.xul
chrome://pippki/content/setp12password.xul
chrome://pippki/content/certViewer.xul
chrome://pippki/content/createCertInfo.xul
chrome://pippki/content/choosetoken.xul

I would only block 2.2 on fixing the crash, but not on implementing support.

Comment 19

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> I confirmed this is a bug of <keygen> support on b2g. The following sample
> cause the same crash.
> 
> https://developer.mozilla.org/en-US/docs/Web/HTML/Element/keygen

I checked on v1.3, somehow, keygen tag was not recognized by gecko. Therefore, nsKeygenFormProcessor was not created.

Comment 20

[Sotaro Ikeda [:sotaro]]

(In reply to Paul Theriault [:pauljt] from comment #17)
> (In reply to bhavana bajaj [:bajaj] from comment #16)
> > Paul, can you help find an owner on this ? Also, I am not sure if we'd block
> > the release on this given this is not a recent regression, any input here?
> 
> Why would say its not a recent regression? It seems to work on 2.1, but not
> 2.2?

My comment is just about nsNSSDialogHelper::openDialog(). It existed long time. And it does not work on b2g gonk as in Comment 18. And direct cause of the regression seems Bug 582297.

Comment 21

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Fabrice Desré [:fabrice] from comment #18)
> 
> I would only block 2.2 on fixing the crash, but not on implementing support.

+1

Comment 22

[Sotaro Ikeda [:sotaro]]

(In reply to Paul Theriault [:pauljt] from comment #21)
> (In reply to Fabrice Desré [:fabrice] from comment #18)
> > 
> > I would only block 2.2 on fixing the crash, but not on implementing support.
> 
> +1

+1

Comment 23

[Aaron Wu]

Pau;/Fabrice,
will we fix it on 2.2 before FC (4/29)?

Comment 24

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Does anyone on this bug have an idea of the options here? I'm unclear what approach we should take:

1. modify nsNSSDialogs.cpp such that it doesnt do attempt to open XUL windows on b2g
2. prevent XUL loading on b2g instead of crashing when this is attempted

Option 2 seems more robust to me (if it is an option), but I don't know where to start or who to ask. Any suggestions?

Comment 25

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

I'm pretty sure all you need to do is copy the file at [1] into the b2g/components folder, remove the function implementations so that you just have stub implementations, and hook it up to the b2g build. Just like [2] but with stubs.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/NSSDialogService.js
[2] https://hg.mozilla.org/mozilla-central/rev/a8a7bc20e2aa

Comment 26

[[:fabrice] Fabrice Desré]

Attachment: keygen-crash.patch

This patch does the absolute minimum to prevent crashing. I think this is also where we would have to hook up the b2g dialog implementation, filed in bug 1157830.

Comment 27

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8596719 [details] [diff] [review]
keygen-crash.patch

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

This looks like a good approach to prevent crashes. In the future, it would be best if we could move away from PSM opening any dialogs at all.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/8ecc2d803e35

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ecc2d803e35

Comment 30

[Verson Xiong (Leave from Mozilla)]

Hi Carsten,
The problem is verified not happen on latest Flame 3.0 build but still exist on latest Flame 2.2 build.
Could you uplift the patch to 2.2 branch? 
thanks!
Flame 2.2 (affected)
Build ID               20150427002504
Gaia Revision          265ca0bc9408c21fc4b25a259fcee7fb642cd06b
Gaia Date              2015-04-24 19:13:28
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1908685d798d
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150427.040113
Firmware Date          Mon Apr 27 04:01:24 EDT 2015
Bootloader             L1TC000118D0

Flame 3.0 (Unaffected)
Build ID               20150427160201
Gaia Revision          0636405f0844bf32451a375b2d61a2b16fe33348
Gaia Date              2015-04-27 16:42:28
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/caf25344f73e
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150427.192938
Firmware Date          Mon Apr 27 19:29:49 EDT 2015
Bootloader             L1TC000118D0

Comment 31

[Carsten Book [:Tomcat]]

Fabrice see comment #30 seems this need a approval request i think

Comment 32

[[:fabrice] Fabrice Desré]

Comment on attachment 8596719 [details] [diff] [review]
keygen-crash.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unclear regression since 2.1
User impact if declined: crash in pages using <keygen>
Testing completed: QA verified on 3.0
Risk to taking this patch (and alternatives if risky): no risk.
String or UUID changes made by this patch: none

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9260b1dbb96d

Comment 34

[Verson Xiong (Leave from Mozilla)]

This issue is verified fixed on latest build of Flame 2.2,he STR is same as Comment 0.
Reproduce rate:0/5
Flame 2.2(Fixed):
Build ID               20150429162504
Gaia Revision          7a9254e35efad3083707dbc23cc954a562a4f873
Gaia Date              2015-04-29 16:38:40
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3cede85ef2ad
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150429.200313
Firmware Date          Wed Apr 29 20:03:24 EDT 2015
Bootloader             L1TC000118D0