596988 - Crash [@ js_ConcatStrings]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Martijn Wargers (dead)]

While testing, I keep getting hit by these kind of crashes.

http://crash-stats.mozilla.com/report/index/bp-f207ec22-5176-4fc1-b135-a931e2100914
0  	xul.dll  	js_ConcatStrings  	 js/src/jsstr.cpp:382
1 	xul.dll 	js::mjit::stubs::Add 	js/src/methodjit/StubCalls.cpp:1216

It would be nice if it could get fixed.

Comment 1

[Mike Shaver (:shaver emeritus)]

Do you know when it started?

Comment 2

[Martijn Wargers (dead)]

Probably when tracemonkey landed or something.

Comment 3

[Alan Pierce]

Attachment: Patch

This patch probably fixes this issue. It fixes a stupid OOM-propagation bug. I'm flagging gal for review, since he reviewed the original ropes patch.

Here's the story to explain the line number (the line is a call to ObtainRopeBuffer, which is forced to be inlined, so the line number info just says that the crash happened in ObtainRopeBuffer), and why this would occur relatively rarely:

-Concat strings is called where at least one argument is already a rope, and its underlying buffer is not big enough, so a new one needs to be malloced.
-malloc inside ObtainRopeBuffer fails, so ObtainRopeBuffer returns NULL.
-The NULL is sent to FinishConcat (instead of being propagated, as it should be), so we return a rope that is otherwise completely valid, except that the underlying buffer of the top node is instead NULL.
-There are no (failed) mallocs until the next call to js_ConcatStrings.
-The next call to js_ConcatStrings calls ObtainRopeBuffer, which tries to read from the NULL buffer, causing a crash.

Comment 4

[Alan Pierce]

I don't have commit access, so someone else needs to commit this.

Comment 5

[Mike Shaver (:shaver emeritus)]

Do we really think that we're in OOM when this hits?  I would hope that'd be rarer than this bug seems to be...

Comment 6

[Mike Shaver (:shaver emeritus)]

(See also bug 597886.)

Comment 7

[Martijn Wargers (dead)]

I was hitting this often while testing bug 581539.

Comment 9

[Brendan Eich [:brendan]]

Don't code else after return. Whoever lands this can fix with rs=me unless Alan wants to do a new patch.

/be

Comment 10

[Rob Crowther]

I'm seeing a crash in js_ConcatStrings when I'm trying to download large files (Go-OO ~180Mb) in the latest nightly on Win XP.

Crash report: http://crash-stats.mozilla.com/report/index/bp-c4440b0a-240b-431a-ac4f-fc6ad2100928 (there's a bunch more, but they all look the same to me)

Comment 11

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/tracemonkey/rev/aa2ca63a88d8

Refactored code to eliminate else-after-return. OK'd by Alan.

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/aa2ca63a88d8