If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Large url crashes [@ malloc_print_stats | @0x0 | getanswer ] Fennec

RESOLVED WONTFIX

Status

()

Firefox for Android
General
P1
critical
RESOLVED WONTFIX
6 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: gbrown)

Tracking

({crash, reproducible, testcase})

Trunk
ARM
Android
crash, reproducible, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [native-crash], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 597452 [details]
testcase

Steps to reproduce:
- Visit testcase, tap on the button (which makes a really large url after the '#' part).

Result: crash

I noticed on various phones that I tested that not only Fennec crashes, but it seems to crash after every restart, unless I clear all data.

Tested on the HTC Desire HD, LG Optimus Black an Samsung Galaxy Nexus (ICS Fennec doesn't give a crash reporter?)

https://crash-stats.mozilla.com/report/index/bp-cf30c609-f296-4751-8aa4-ab8872120215
0 	libmozglue.so 	malloc_print_stats 	memory/jemalloc/jemalloc.c:2164
1 		@0x0 	
2 	libmozglue.so 	getanswer 	other-licenses/android/getaddrinfo.c:1419
3 	libxul.so 	nsRegion::SetToElements 	gfx/src/nsRegion.cpp:330
4 	libxul.so 	nsRegion::SetEmpty 	gfx/src/nsRegion.h:173
5 	libxul.so 	nsRegion::SubRegion 	gfx/src/nsRegion.cpp:1070
6 		@0x45abbf86 	
7 	libxul.so 	nsDisplayListBuilder::SubtractFromVisibleRegion 	nsRegion.h:107
8 	libxul.so 	nsDisplayList::ComputeVisibilityForSublist 	nsTArray.h:480

Updated

6 years ago
Whiteboard: [native-crash]
(Reporter)

Comment 1

6 years ago
This was on current mozilla-central nightly.
tracking-fennec: --- → ?

Updated

6 years ago
Priority: -- → P1

Updated

6 years ago
tracking-fennec: ? → ---
Keywords: reproducible
(Assignee)

Comment 2

6 years ago
To be clear, the "really large url" in this test case is over 2 million characters in length. There are no problems if the length is reduced to, for example, 4000 characters.

The test case consistently crashes for me, but I have not seen this particular stack trace yet in my testing. Instead, I see crashes on database operations in BrowserProvider.updateHistory(). I don't fully understand what fails in updateHistory, but if I skip that code, Fennec still crashes with an OOM exception when the URI is copied or manipulated.


An obvious solution is to restrict the size of the URL by truncating input -- is that a reasonable approach?

There is some info on max url lengths for browsers here: http://www.boutell.com/newfaq/misc/urllength.html
Assignee: nobody → gbrown
limiting the URL to something less than 2 million chars sounds fine to me. let's just be sure that using a shorter URL doesn't just take longer to crash, after repeatably loading the URL.

Can you test 64k, 128k, 256k limits and see how those work?
(Assignee)

Comment 4

6 years ago
Created attachment 606807 [details] [diff] [review]
patch to truncate uri in json - for discussion only

This patch truncates the uri and documentURI fields of JSON messages and avoids the crashes we were seeing with this test case. 

2 problems remain:
1. Clicking the button additional times still results in OOM. I think this is because of Gecko allocations (the script keeps doubling its string regardless of this truncation, so the 2nd click generates a 4MB string, the 3rd an 8MB string, etc).
2. Even on the first click, I see evidence of database operations on the 2MB data; I don't know where this is coming from:

E/CursorWindow(32279): need to grow: mSize = 2097152, size = 2097179, freeSpace(
) = 2092635, numRows = 4
E/CursorWindow(32279): not growing since there are already 4 row(s), max size 20
97152
E/Cursor  (32279): Failed allocating 2097179 bytes for text/blob at 3,1
D/Cursor  (32279): finish_program_and_get_row_count row 0


Looking for suggestions for a better (earlier) place to detect and truncate very large input...
(Assignee)

Comment 5

6 years ago
Created attachment 609794 [details] [diff] [review]
truncate uri in browser.js

The approach used here is straight-forward: truncate uri fields before sending to Java. With this patch, the test case passes, and the button can be clicked about 3 times (on my Galaxy S) before the OS kills Fennec for excessive memory use. (Fennec does not report an exception...it is killed cleanly.) Note that the test is additive: first click creates a 2MB string, second click a 4MB string, 3rd click a 8MB string...so this result seems reasonable.

I chose a limit of 2048 characters because that limit is already used in, for example, http://hg.mozilla.org/mozilla-central/file/0ff816e5e992/dom/src/offline/nsDOMOfflineResourceList.cpp#l84 ... but much larger values (eg. 256K) are equally effective in addressing this bug.

I am concerned that this solution seems "fragile" - truncation code is duplicated and needs to be carried over to any new messages containing uri fields. Suggestions for a different approach welcome!
Attachment #606807 - Attachment is obsolete: true
Attachment #609794 - Flags: review?(mark.finkle)
Since the patch to limit the URL still allows a crash - an expected crash given the huge string - I think we should WONTFIX this. Thoughts?
I'm going to WONTFIX after a quick discussion on IRC and looking at the URL limit in Firefox and other browsers:
http://www.boutell.com/newfaq/misc/urllength.html

Getting OOM killed for trying to allocate more than the devices memory will happen.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
Attachment #609794 - Flags: review?(mark.finkle)
You need to log in before you can comment on or make changes to this bug.