Add <iframe mozbrowser> securitychange event

RESOLVED FIXED

Status

RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

As I understand, Ben would be very happy if I could give him a lock icon.
Hm, can't test EV certs due to bug 458727.
(Assignee)

Updated

7 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Updated

7 years ago
Blocks: 693515
Sigh, turns out this is more complicated than I expected because iframes don't have an instance of nsSecureBrowserUIImpl instantiated for them.
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)
> 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.
Created attachment 632071 [details] [diff] [review]
Part 1: Implement securityChange event in <iframe mozbrowser>. (asserts)
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.
Attachment #632071 - Flags: feedback?(bsmith)
(Assignee)

Updated

7 years ago
Attachment #632071 - Attachment description: Part 1: Implement securityChange event in <iframe mozbrowser>. → Part 1: Implement securityChange event in <iframe mozbrowser>. (asserts)
Keywords: dev-doc-needed
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.
> 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
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.
(Assignee)

Updated

7 years ago
Attachment #632071 - Flags: feedback?(bsmith) → review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #632072 - Flags: review?(bugs)
The EV bug is 764496.  Once that is fixed, I think EV should Just Work in mozbrowser.
(Assignee)

Updated

7 years ago
Summary: Add <iframe mozbrowser> security-change event → Add <iframe mozbrowser> securitychange event
New patch based atop bug 764248 in a moment.
Depends on: 764248
Created attachment 632845 [details] [diff] [review]
Part 1, v1.1 (rebased atop bug 764248)

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

Updated

7 years ago
Attachment #632071 - Attachment is obsolete: true
Attachment #632071 - Flags: review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #632845 - Flags: review?(bugs)
Attachment #632845 - Flags: review?(bugs) → review+
Attachment #632072 - Flags: review?(bugs) → review+
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
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.
(Assignee)

Updated

7 years ago
Depends on: 766586
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
https://hg.mozilla.org/mozilla-central/rev/a81245912f3f
https://hg.mozilla.org/mozilla-central/rev/3c6c55befb22
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Depends on: 768001
(Assignee)

Updated

7 years ago
Depends on: 764496
Depends on: 774807
Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/mozbrowsersecuritychange
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.