Closed Bug 1607159 Opened 10 months ago Closed 9 months ago

Crash in [@ mozilla::ipc::IProtocol::ChannelSend] called from Document::GetFailedCertSecurityInfo

Categories

(Firefox :: Security, defect, P1)

x86_64
All
defect

Tracking

()

VERIFIED FIXED
Firefox 74
Tracking Status
firefox-esr68 --- unaffected
firefox72 --- wontfix
firefox73 --- verified
firefox74 --- verified

People

(Reporter: gsvelto, Assigned: johannh)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug is for crash report bp-e7a94e34-28c0-4ecf-8b4b-ff1410200105.

Top 10 frames of crashing thread:

0 xul.dll mozilla::ipc::IProtocol::ChannelSend ipc/glue/ProtocolUtils.cpp:487
1 xul.dll mozilla::dom::PContentChild::SendIsSecureURI ipc/ipdl/PContentChild.cpp:2372
2 xul.dll mozilla::dom::Document::GetFailedCertSecurityInfo dom/base/Document.cpp:1804
3 xul.dll mozilla::dom::Document_Binding::getFailedCertSecurityInfo dom/bindings/DocumentBinding.cpp:5635
4 xul.dll mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3151
5 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:544
6 xul.dll Interpret js/src/vm/Interpreter.cpp:3038
7 xul.dll js::RunScript js/src/vm/Interpreter.cpp:424
8 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:580
9 xul.dll js::jit::DoCallFallback js/src/jit/BaselineIC.cpp:2905

This is not a new crash, it seems to have started in version 69.0. It seems to involve failedCertChain so maybe it's related to bug 1555438? Just a guess, I don't know this code very well.

It's happening mostly on Windows but there's Linux crashes too with the same stack so it's probably not platform-specific.

Maybe Document::GetFailedCertSecurityInfo needs to null check cc?

Summary: Crash in [@ mozilla::ipc::IProtocol::ChannelSend] → Crash in [@ mozilla::ipc::IProtocol::ChannelSend] called from Document::GetFailedCertSecurityInfo

The regression speculation in comment 0 looks plausible to me.

Keywords: regression
Regressed by: 1555438

I guess this is getting called from the parent process, where we don't have ContentChild? I don't know what this code should do in that case.

Yup, turns out these pages are crashing with e10s disabled now. It's not great but I have to say we've come a long way for this to go unnoticed for so long.

Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: -- → P1
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa3c61d28ac8
Without e10s, don't crash the parent process in Document::GetFailedCertSecurityInfo. r=baku
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74

Please nominate this for Beta approval when you get a chance.

Flags: needinfo?(jhofmann)

Comment on attachment 9120752 [details]
Bug 1607159 - Without e10s, don't crash the parent process in Document::GetFailedCertSecurityInfo. r=baku

Beta/Release Uplift Approval Request

  • User impact if declined: Browsers with e10s disabled will crash to desktop when hitting certificate error pages
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Run Firefox with e10s disabled (afaik this needs to be specified as a command line flag nowadays) and visit https://expired.badssl.com/
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This adds a few lines of well-understood code that only takes effect for non-e10s (or any other case where cert error pages are loaded in the parent)
  • String changes made/needed: None
Flags: needinfo?(jhofmann)
Attachment #9120752 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Hello! Reproduced with Firefox 74.0a1 (20200106215403) on Windows 10x64 by setting "set/export MOZ_FORCE_DISABLE_E10S=1" as an environment variable, and visiting https://expired.badssl.com/. Here is the crash report.
The issue is verified fixed with Firefox 74.0a1 (20200115213621) on Windows 10x64, macOS 10.15 and Ubuntu 18.04.

Flags: qe-verify+

Adding back the qe+ flag. Sorry!

Flags: qe-verify+

Comment on attachment 9120752 [details]
Bug 1607159 - Without e10s, don't crash the parent process in Document::GetFailedCertSecurityInfo. r=baku

Fixes a crash for situations when e10s is disabled. Approved for 73.0b6.

Attachment #9120752 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified on 73.0b6 (20200116232741) on Windows 10x64, macOS 10.15 and Ubuntu 18.04.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.