1722925 - Crash in [@ OOM | unknown | NS_ABORT_OOM | nsDataHandler::CreateNewURI]

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Emily Toop (:fluffyemily)]

Crash report: https://crash-stats.mozilla.org/report/index/ac3d5ac2-5bb0-4c3e-bf15-d19920210729

Reason: SIGSEGV /SEGV_MAPERR

Top 10 frames of crashing thread:

0 libxul.so NS_ABORT_OOM xpcom/base/nsDebugImpl.cpp:618
1 libxul.so nsDataHandler::CreateNewURI netwerk/protocol/data/nsDataHandler.cpp:78
2 libxul.so NS_NewURI netwerk/base/nsNetUtil.cpp:1827
3 libxul.so NS_NewURI netwerk/base/nsNetUtil.cpp:1699
4 libxul.so NS_NewURI netwerk/base/nsNetUtil.cpp:1719
5 libxul.so mozilla::dom::HTMLImageElement::LoadSelectedImage dom/html/HTMLImageElement.cpp:905
6 libxul.so mozilla::dom::HTMLImageElement::AfterMaybeChangeAttr dom/html/HTMLImageElement.cpp:480
7 libxul.so mozilla::dom::HTMLImageElement::AfterSetAttr dom/html/HTMLImageElement.cpp:322
8 libxul.so mozilla::dom::Element::SetAttrAndNotify dom/base/Element.cpp:2502
9 libxul.so mozilla::dom::Element::SetAttr dom/base/Element.cpp:2358

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Kicking this over to Necko:
In GeckoView we set a maximum length for data: URIs when they are passed in from the app, however for webcompat reasons we do not limit the URI length if the URI comes from web content. Having said that, crashing every time we encounter an excessively long data: URI is less than ideal.

Can we make memory allocations for data: URIs fallible on Android?

Comment 3

[Valentin Gosu [:valentin] (he/him)]

We already use fallible allocation for most of the parsing jobs, but it seems passing the spec as an argument to NS_MutateURI.Apply creates a copy of the string instead, so that needs to be fixed.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1722925 - Remove NS_MutatorMethod in favor of templated nsIURIMutator::Apply r=#necko

This basically reverts the changes in 5caa81103c00 (bug 1435671). In that bug
we switched from having a templated method to using a templated function
that returned a lambda because the templated method caused a binary size
regression on windows (MSVC). Since Firefox 67 we no longer support MSVC.
Using a lambda also required capturing the arguments by value, so it was
slightly inefficient.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1722925 - Avoid unnecessary string copies by nsCString constructor r=#necko

Since the Find method does not exist on nsACString, I had to use std::search
to find the "data:" substring in the spec.

Depends on D122081

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e2a98aeca5c
Remove NS_MutatorMethod in favor of templated nsIURIMutator::Apply r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/95a6eacbef27
Avoid unnecessary string copies by nsCString constructor r=necko-reviewers,kershaw

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25844f5fe51d
Backed out 2 changesets for causing reftest failures in color_quads. CLOSED TREE

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c94d9c9c373
Remove NS_MutatorMethod in favor of templated nsIURIMutator::Apply r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/a7bb8289f02e
Avoid unnecessary string copies by nsCString constructor r=necko-reviewers,kershaw CLOSED TREE

Comment 9

[Iulian Moraru]

This was backed out by mistake. I will reland it now.

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/0c94d9c9c373
https://hg.mozilla.org/mozilla-central/rev/a7bb8289f02e

Comment 11

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

The patch landed in nightly and beta is affected.
:valentin, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9235356 [details]
Bug 1722925 - Remove NS_MutatorMethod in favor of templated nsIURIMutator::Apply r=#necko

Beta/Release Uplift Approval Request

• User impact if declined: Potential OOM crash when website is using large data URLs
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): These patches optimize the way we call nsIURI methods to avoid copying nsCStrings.
  Behaviour should not change at all. While I don't expect this patch to completely fix data URI OOMs, I think it should improve things somewhat.
• String changes made/needed:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9235356 [details]
Bug 1722925 - Remove NS_MutatorMethod in favor of templated nsIURIMutator::Apply r=#necko

Approved for 92.0b5.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c2a3841abfaf
https://hg.mozilla.org/releases/mozilla-beta/rev/9f65b8c7ef9d