1721897 - [Fission] Crash in [@ mozilla::dom::PaymentRequest::Constructor]

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, defect)

Description

[Andrew McCreight [:mccr8]]

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/b9e9ccc8-3cbc-4eb2-9eda-2de310210503

Reason: SIGSEGV /SEGV_MAPERR

Top 10 frames of crashing thread:

0 libxul.so mozilla::dom::PaymentRequest::Constructor dom/payments/PaymentRequest.cpp:615
1 libxul.so mozilla::dom::PaymentRequest_Binding::_constructor s3:gecko-generated-sources-l1:a08467c4fe2aec7311cca1f2ea239445579d0d97059f667a0f0f8650a7a928024209e6cddf39c97f40b964eeed4d2c1d349902cc6f22f4b0ab0738713a7c39d3/dom/bindings/PaymentRequestBinding.cpp::3266
2 libxul.so Interpret js/src/vm/Interpreter.cpp:3238
3 libxul.so js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:554
4 libxul.so js::fun_apply js/src/vm/JSFunction.cpp:1167
5 libxul.so js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:522
6 libxul.so Interpret js/src/vm/Interpreter.cpp:3248
7 libxul.so js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:554
8 libxul.so js::jit::DoCallFallback js/src/jit/BaselineIC.cpp:1834
9  @0x18ecc125dd87 

23 crashes in the last 3 months. 100% of them have Fission enabled. This is a feature that is disabled by default. Maybe it isn't Fission-compatible?

The crash is a null deref on the third line here:

RefPtr<Document> topLevelDoc = doc->GetTopLevelContentDocumentIfSameProcess();
MOZ_ASSERT(topLevelDoc);
nsCOMPtr<nsIPrincipal> topLevelPrincipal = topLevelDoc->NodePrincipal();

Maybe we could make this method return null instead of crash.

Comment 1

[Chris Peterson [:cpeterson]]

This is a feature that is disabled by default. Maybe it isn't Fission-compatible?

@ Jens: who owns PaymentRequest?

The feature is currently disabled by default, but some users appear to have manually enabled it with the dom.payments.request.enabled pref. (We know because we're receiving some crash reports from them.) PaymentRequest is known to be broken with Fission. The PaymentRequest owner should probably force-disable PaymentRequest when Fission is enabled so those users don't crash or expose themselves to security bugs caused by PaymentRequest running in the wrong process.

Comment 2

[Jens Stutte [:jstutte]]

(In reply to Andrew McCreight [:mccr8] from comment #0)

The crash is a null deref on the third line here:

RefPtr<Document> topLevelDoc = doc->GetTopLevelContentDocumentIfSameProcess();
MOZ_ASSERT(topLevelDoc);
nsCOMPtr<nsIPrincipal> topLevelPrincipal = topLevelDoc->NodePrincipal();

Maybe we could make this method return null instead of crash.

There seem to be several nullptr exit points from GetTopLevelContentDocumentIfSameProcess and in all other places the callers implement explicit checks if it returned a top level document. So yes, we probably should just null-check here, too. And we should replicate something similar to this error behavior to inform the caller.

(In reply to Chris Peterson [:cpeterson] from comment #1)

@ Jens: who owns PaymentRequest?

Well, the Workers & Storage team, though we have no active development ongoing here right now. But maintenance is here.

PaymentRequest is known to be broken with Fission.

Do we have other crashes with fission or is this the only one known? In any case I assume there might be also non-fission edge cases for this one that could trigger here.

The PaymentRequest owner should probably force-disable PaymentRequest when Fission is enabled so those users don't crash or expose themselves to security bugs caused by PaymentRequest running in the wrong process.

If there are other, different crashes - maybe? But I assume, Fission is just an amplifier of otherwise rare but possible edge cases, so either we should unship PaymentRequest entirely or fix them.

Eden, what do you think about this specific one?

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Jens Stutte [:jstutte] from comment #2)

If there are other, different crashes - maybe? But I assume, Fission is just an amplifier of otherwise rare but possible edge cases, so either we should unship PaymentRequest entirely or fix them.

This is the only Fission crash in Payment code that I have seen in Bugzilla or crash reports.

I don't think this bug is Fission MVP blocker. I just wanted to bring it to your attention, in case you want to force-disable it when Fission is enabled.

Another option might be to change the dom.payments.request.enabled pref name, so PaymentRequest could be disabled again for any users who had manually enabled the old pref in the past. Renaming the pref might be easier than adding a special case checking for Fission. It would also benefit non-Fission users with the old pref enabled.

Comment 4

[Eden Chuang[:edenchuang]]

Attachment: Bug 1721897 - Getting the top same process parent document's principal for payment request.

Comment 5

[Eden Chuang[:edenchuang]]

I agree this bug is not a fission MVP blocker.

In general, according to the PaymentRequest API spec, PaymentRequest should not be related to fission, because it doesn't need any information from its parent or children documents. So basically, I will say any PaymentRequest bugs should not be a Fission MVP blocker.

So, why did we meet this crash?
The root cause is we want to show where this PaymentRequest is from in our UI implementation.
And in the original UI design, we show the top-level document's origin to users.
And we use "Document::GetTopLevelContentDocumentIfSameProcess()" to get this information.
However, "Document::GetTopLevelContentDocumentIfSameProcess()" always return nullptr for cross-origin iframe in fission world, so we deref with a nullptr.

The interesting thing is PaymentRequest API spec doesn't define any UI spec on how browsers should interact with users because it shouldn't.

What can we do?
I think where the PaymentRequest from is still important for users, but it should point out which site but not which top window.
So I wrote a patch to get the top same process parent content document instead of the top-level document.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d97076442db2
Getting the top same process parent document's principal for payment request. r=kmag

Comment 7

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/d97076442db2

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 9

[Chris Peterson [:cpeterson]]

Setting status-firefox93=wontfix. Crash volume is very low, so we don't need to uplift this fix to Beta 93.