1418810 - Crash in OOM | large | NS_ABORT_OOM | mozilla::net::nsSimpleURI::SetPathQueryRefEscaped

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-f72383be-eb2f-4c54-a8c3-8c7e60171119.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll NS_ABORT_OOM xpcom/base/nsDebugImpl.cpp:620
1 xul.dll mozilla::net::nsSimpleURI::SetPathQueryRefEscaped netwerk/base/nsSimpleURI.cpp:472
2 xul.dll mozilla::net::nsSimpleURI::SetSpec netwerk/base/nsSimpleURI.cpp:309
3 xul.dll nsDataHandler::NewURI netwerk/protocol/data/nsDataHandler.cpp:100
4 xul.dll mozilla::net::nsIOService::NewURI netwerk/base/nsIOService.cpp:697
5 xul.dll NS_NewURI netwerk/base/nsNetUtil.cpp:1782
6 xul.dll NS_NewURI netwerk/base/nsNetUtil.cpp:1795
7 xul.dll NS_NewURI netwerk/base/nsNetUtil.cpp:1815
8 xul.dll nsImageLoadingContent::StringToURI dom/base/nsImageLoadingContent.cpp:1265
9 xul.dll nsImageLoadingContent::LoadImage dom/base/nsImageLoadingContent.cpp:883

=============================================================

crash reports with this signature are newly showing up on fennec in version 57 & on firefox desktop since 58.

Comment 1

[Honza Bambas (:mayhemer)]

This should be a fallible assignment at https://dxr.mozilla.org/mozilla-central/rev/b056526be38e96b3e381b7e90cd8254ad1d96d9d/netwerk/base/nsSimpleURI.cpp#472

Comment 2

[Valentin Gosu [:valentin] (PTO until 28 feb)]

Attachment: Use fallible assign to prevent OOM

MozReview-Commit-ID: LU1RbkUIBZz

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8930604 [details] [diff] [review]
Use fallible assign to prevent OOM

Review of attachment 8930604 [details] [diff] [review]:
-----------------------------------------------------------------

This is good, but question is what happens when we hit this error, have you tried?

Comment 4

[Valentin Gosu [:valentin] (PTO until 28 feb)]

Really hard to make it OOM precisely in that place.
I can run a script that creates a lot of huge URIs, but I don't have a reproducible test.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4873b451e40f
Use fallible assign to prevent OOM. r=mayhemer

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4873b451e40f

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #4)
> Really hard to make it OOM precisely in that place.
> I can run a script that creates a lot of huge URIs, but I don't have a
> reproducible test.

You can just return an error all the time or instrument properly, catch in a debugger and switch to return an error.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Not sure the crash volume on 57 warrants keeping it on the radar for possible dot release inclusion, but a Beta approval request would probably be a good idea.

Comment 9

[Valentin Gosu [:valentin] (PTO until 28 feb)]

Comment on attachment 8930604 [details] [diff] [review]
Use fallible assign to prevent OOM

Approval Request Comment
[Feature/Bug causing the regression]:
Unknown.
[User impact if declined]:
OOM
[Is this code covered by automated tests?]:
No
[Has the fix been verified in Nightly?]:
Yes
[Needs manual test from QE? If yes, steps to reproduce]: 
No. It is difficult to reproduce by manual testing.
[List of other uplifts needed for the feature/fix]:
none
[Is the change risky?]:
No.
[Why is the change risky/not risky?]:
It just makes an allocation fallible, and returns an error otherwise.
[String changes made/needed]:
none.

Comment 10

[Gerry Chang [:gchang]]

Comment on attachment 8930604 [details] [diff] [review]
Use fallible assign to prevent OOM

Fix a crash. Beta58+.

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b863167623e2