Closed Bug 1418810 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Networking, defect, P2, critical)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: philipp, Assigned: valentin)

References

Details

(Keywords: crash, regression, Whiteboard: [necko-triaged])

Crash Data

Attachments

(1 file)

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.
This should be a fallible assignment at https://dxr.mozilla.org/mozilla-central/rev/b056526be38e96b3e381b7e90cd8254ad1d96d9d/netwerk/base/nsSimpleURI.cpp#472
Priority: -- → P2
Whiteboard: [necko-triaged]
Assignee: nobody → valentin.gosu
MozReview-Commit-ID: LU1RbkUIBZz
Attachment #8930604 - Flags: review?(honzab.moz)
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?
Attachment #8930604 - Flags: review?(honzab.moz) → review+
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.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4873b451e40f
Use fallible assign to prevent OOM. r=mayhemer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4873b451e40f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(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.
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.
Flags: needinfo?(valentin.gosu)
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.
Flags: needinfo?(valentin.gosu)
Attachment #8930604 - Flags: approval-mozilla-beta?
Comment on attachment 8930604 [details] [diff] [review]
Use fallible assign to prevent OOM

Fix a crash. Beta58+.
Attachment #8930604 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
See Also: → 1426996
You need to log in before you can comment on or make changes to this bug.