763694 - Add <iframe mozbrowser> securitychange event

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

As I understand, Ben would be very happy if I could give him a lock icon.

Comment 1

[Justin Lebar (not reading bugmail)]

Hm, can't test EV certs due to bug 458727.

Comment 2

[Justin Lebar (not reading bugmail)]

Sigh, turns out this is more complicated than I expected because iframes don't have an instance of nsSecureBrowserUIImpl instantiated for them.

Comment 3

[Justin Lebar (not reading bugmail)]

I get the following assertion when I try to read the is-EV-cert property OOP:

> [Child 29324] ###!!! ASSERTION: nsSSLStatus has null mServerCert or was called in the content process: 'Error', file ../../../../../src/security/manager/ssl/src/nsIdentityChecking.cpp, line 1085
> nsSSLStatus::GetIsExtendedValidation(bool*) (/home/jlebar/code/moz/ff-git/debug/security/manager/ssl/src/../../../../../src/security/manager/ssl/src/nsIdentityChecking.cpp:1086)
> nsSecureBrowserUIImpl::EvaluateAndUpdateSecurityState(nsIRequest*, nsISupports*, bool) (/home/jlebar/code/moz/ff-git/debug/security/manager/boot/src/../../../../../src/security/manager/boot/src/nsSecureBrowserUIImpl.cpp:502)
> nsSecureBrowserUIImpl::OnLocationChange(nsIWebProgress*, nsIRequest*, nsIURI*, unsigned int) (/home/jlebar/code/moz/ff-git/debug/security/manager/boot/src/../../../../../src/security/manager/boot/src/nsSecureBrowserUIImpl.cpp:1613)
> nsDocLoader::FireOnLocationChange(nsIWebProgress*, nsIRequest*, nsIURI*, unsigned int) (/home/jlebar/code/moz/ff-git/debug/uriloader/base/../../../src/uriloader/base/nsDocLoader.cpp:1390)
> nsDocShell::CreateContentViewer(char const*, nsIRequest*, nsIStreamListener**) (/home/jlebar/code/moz/ff-git/debug/docshell/base/../../../src/docshell/base/nsDocShell.cpp:7633)
> nsDSURIContentListener::DoContent(char const*, bool, nsIRequest*, nsIStreamListener**, bool*) (/home/jlebar/code/moz/ff-git/debug/docshell/base/../../../src/docshell/base/nsDSURIContentListener.cpp:132)
> nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) (/home/jlebar/code/moz/ff-git/debug/uriloader/base/../../../src/uriloader/base/nsURILoader.cpp:677)
> nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) (/home/jlebar/code/moz/ff-git/debug/uriloader/base/../../../src/uriloader/base/nsURILoader.cpp:374)
> nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*) (/home/jlebar/code/moz/ff-git/debug/uriloader/base/../../../src/uriloader/base/nsURILoader.cpp:262)
> mozilla::net::HttpChannelChild::OnStartRequest(nsHttpResponseHead const&, bool const&, nsHttpHeaderArray const&, bool const&, bool const&, unsigned int const&, nsCString const&, nsCString const&, PRNetAddr const&, PRNetAddr const&) (/home/jlebar/code/moz/ff-git/debug/netwerk/protocol/http/../../../../src/netwerk/protocol/http/HttpChannelChild.cpp:270)
> mozilla::net::HttpChannelChild::RecvOnStartRequest(nsHttpResponseHead const&, bool const&, nsHttpHeaderArray const&, bool const&, bool const&, unsigned int const&, nsCString const&, nsCString const&, PRNetAddr const&, PRNetAddr const&) (/home/jlebar/code/moz/ff-git/debug/netwerk/protocol/http/../../../../src/netwerk/protocol/http/HttpChannelChild.cpp:231)
> mozilla::net::PHttpChannelChild::OnMessageReceived(IPC::Message const&) (/home/jlebar/code/moz/ff-git/debug/ipc/ipdl/PHttpChannelChild.cpp:531)
> mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) (/home/jlebar/code/moz/ff-git/debug/ipc/ipdl/PContentChild.cpp:1641)
> mozilla::ipc::AsyncChannel::OnDispatchMessage(IPC::Message const&) (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/AsyncChannel.cpp:463)
> mozilla::ipc::RPCChannel::OnMaybeDequeueOne() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/RPCChannel.cpp:404)
> void DispatchToMethod<mozilla::ipc::RPCChannel, bool (mozilla::ipc::RPCChannel::*)()>(mozilla::ipc::RPCChannel*, bool (mozilla::ipc::RPCChannel::*)(), Tuple0 const&) (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/chromium/src/base/tuple.h:384)
> RunnableMethod<mozilla::ipc::RPCChannel, bool (mozilla::ipc::RPCChannel::*)(), Tuple0>::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/chromium/src/base/task.h:308)
> mozilla::ipc::RPCChannel::RefCountedTask::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../dist/include/mozilla/ipc/RPCChannel.h:430)
> mozilla::ipc::RPCChannel::DequeueTask::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../dist/include/mozilla/ipc/RPCChannel.h:453)
> MessageLoop::RunTask(Task*) (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:319)
> MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:329)
> MessageLoop::DoWork() (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:426)
> mozilla::ipc::DoWorkRunnable::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/MessagePump.cpp:43)
> nsThread::ProcessNextEvent(bool, bool*) (/home/jlebar/code/moz/ff-git/debug/xpcom/threads/../../../src/xpcom/threads/nsThread.cpp:624)
> NS_ProcessNextEvent_P(nsIThread*, bool) (/home/jlebar/code/moz/ff-git/debug/xpcom/build/nsThreadUtils.cpp:213)
> mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/MessagePump.cpp:79)
> mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/MessagePump.cpp:209)
> MessageLoop::RunInternal() (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:209)
> MessageLoop::RunHandler() (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:202)
> MessageLoop::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:175)
> nsBaseAppShell::Run() (/home/jlebar/code/moz/ff-git/debug/widget/xpwidgets/../../../src/widget/xpwidgets/nsBaseAppShell.cpp:165)
> XRE_RunAppShell (/home/jlebar/code/moz/ff-git/debug/toolkit/xre/../../../src/toolkit/xre/nsEmbedFunctions.cpp:641)
> mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/MessagePump.cpp:194)
> MessageLoop::RunInternal() (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:209)
> MessageLoop::RunHandler() (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:202)
> MessageLoop::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:175)
> XRE_InitChildProcess (/home/jlebar/code/moz/ff-git/debug/toolkit/xre/../../../src/toolkit/xre/nsEmbedFunctions.cpp:484)
> main (/home/jlebar/code/moz/ff-git/debug/ipc/app/../../../src/ipc/app/MozillaRuntimeMain.cpp:48)

Comment 4

[Justin Lebar (not reading bugmail)]

> I get the following assertion when I try to read the is-EV-cert property OOP:

Actually, I can a backtrace -- that assertion happens before I try to read the is-ev property.

Brian, can you comment on how I'm supposed to handle this?  I'm clearly not doing things as expected.  I'll post my patch in a moment.

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Part 1: Implement securityChange event in <iframe mozbrowser>. (asserts)

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Part 2: Tests

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 632071 [details] [diff] [review]
Part 1: Implement securityChange event in <iframe mozbrowser>. (asserts)

f? because I'd like to know what's the right thing to do here.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

In general, PSM must only be used in chrome processes because NSS is only used in chrome processes. If you want to use nsSecureBrowserUIImpl as-is, then the nsSecureBrowserUIImpl needs to be instantiated in the chrome process, and then you need to proxy the information you need from the nsSecureBrowserUIImpl to the content process.

In other words, don't instantiate nsSecureBrowserUIImpl within a content process.

Comment 9

[Justin Lebar (not reading bugmail)]

> If you want to use nsSecureBrowserUIImpl as-is, then the nsSecureBrowserUIImpl needs to be 
> instantiated in the chrome process

The relevant window exists only in the content process, so I don't see how that would work.

It looks like XUL Fennec does basically the same thing as this patch: It listens to securitychange events in the child process.  I don't see it explicitly initializing the SecureBrowserUI, but I expect that's happening somehow, because SecureBrowserUI is the only thing I can find that triggers securitychange events.

Maybe I'm misunderstanding this?  http://hg.mozilla.org/mozilla-central/file/bdbed29aaaa6/mobile/xul/chrome/content/bindings/browser.js#l82

Comment 10

[Justin Lebar (not reading bugmail)]

Brian and I talked on IRC and figured out that, aside from the EV problem, this is all fine.

I'll file a separate bug on the EV business.  It's harmless for our purposes; just means that we'll get spew in debug builds and that we won't be able to detect EV certs.

I'm going to continue sending |event.detail.extendedValidation == false|, so that the API is clear even if it doesn't work.  I'll add a comment to the patch pointing to the new EV bug, but aside from that, this is ready for review.

Comment 11

[Justin Lebar (not reading bugmail)]

The EV bug is 764496.  Once that is fixed, I think EV should Just Work in mozbrowser.

Comment 12

[Justin Lebar (not reading bugmail)]

New patch based atop bug 764248 in a moment.

Comment 13

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v1.1 (rebased atop bug 764248)

I guess this wasn't really necessary; it's a change only to irrelevant context.

Comment 14

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19ae28cad1da
https://hg.mozilla.org/integration/mozilla-inbound/rev/453c74176a35

Comment 15

[Ed Morley [:emorley]]

Backed out for native Android failures:
(This bug's push didn't get builds, next one did)
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4a44ebe3e8ff

https://tbpl.mozilla.org/php/getParsedLog.php?id=12816346&tree=Mozilla-Inbound
{
699 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/browser-element/mochitest/test_browserElement_inproc_SecurityChange.html | Test timed out.
916 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/browser-element/mochitest/test_browserElement_oop_SecurityChange.html | Test timed out.
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cb8b0d62bbd

Comment 16

[Justin Lebar (not reading bugmail)]

Given the choice between wasting a day fixing this and, alternatively, landing this now for B2G, that's an easy choice.  We can figure out what the heck is wrong with native Android later.  Unless anyone has objections, I'll disable this test on native Android and push again.

Comment 17

[Justin Lebar (not reading bugmail)]

Filed bug 766586 on things not working on native android, and pushed again with the tests disabled there.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a81245912f3f
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c6c55befb22

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a81245912f3f
https://hg.mozilla.org/mozilla-central/rev/3c6c55befb22

Comment 19

[Jeremie Patonnier :Jeremie]

Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/mozbrowsersecuritychange