The default bug view has changed. See this FAQ.

Add <iframe mozbrowser> securitychange event

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
As I understand, Ben would be very happy if I could give him a lock icon.
(Assignee)

Comment 1

5 years ago
Hm, can't test EV certs due to bug 458727.
(Assignee)

Updated

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

Updated

5 years ago
Blocks: 693515
(Assignee)

Comment 2

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

Comment 3

5 years ago
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)
(Assignee)

Comment 4

5 years ago
> 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.
(Assignee)

Comment 5

5 years ago
Created attachment 632071 [details] [diff] [review]
Part 1: Implement securityChange event in <iframe mozbrowser>. (asserts)
(Assignee)

Comment 6

5 years ago
Created attachment 632072 [details] [diff] [review]
Part 2: Tests
(Assignee)

Comment 7

5 years ago
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

5 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.
(Assignee)

Comment 9

5 years ago
> 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
(Assignee)

Comment 10

5 years ago
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

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

Updated

5 years ago
Attachment #632072 - Flags: review?(bugs)
(Assignee)

Comment 11

5 years ago
The EV bug is 764496.  Once that is fixed, I think EV should Just Work in mozbrowser.
(Assignee)

Updated

5 years ago
Summary: Add <iframe mozbrowser> security-change event → Add <iframe mozbrowser> securitychange event
(Assignee)

Comment 12

5 years ago
New patch based atop bug 764248 in a moment.
Depends on: 764248
(Assignee)

Comment 13

5 years ago
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

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

Updated

5 years ago
Attachment #632845 - Flags: review?(bugs)
Attachment #632845 - Flags: review?(bugs) → review+
Attachment #632072 - Flags: review?(bugs) → review+
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/19ae28cad1da
https://hg.mozilla.org/integration/mozilla-inbound/rev/453c74176a35
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
(Assignee)

Comment 16

5 years ago
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

5 years ago
Depends on: 766586
(Assignee)

Comment 17

5 years ago
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: 5 years ago
Resolution: --- → FIXED
Depends on: 768001
(Assignee)

Updated

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