393874 - Hang [@ js_MinimizeDependentStrings] with gczeal, e4x comment

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

This hangs the JS shell:

gczeal(2);
void <b><x/><!----></b>;

Comment 1

[Brian Crowder]

We get 100 frames deep and then hang in the loop on jsstr.c:91

Looking at this now, unless anyone else sees it instantly.

Comment 2

[Jesse Ruderman]

Still happens on trunk.

Brian, did you figure this out?  I'd like to at least know whether it's a GC hazard / potential memory safety bug.

Comment 3

[Brian Crowder]

No, I didn't make much progress on it and don't have much time to spend on it at the moment.  Someone else should own.

Comment 4

[Igor Bukanov]

I will have a look.

Comment 5

[Igor Bukanov]

I can not reproduce the bug on Linux with the current tip. Neither pasting the command directly into the shell or via executing through a file causes any hangs.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

This is sort of a GC hazard. In FoldXMLConstants we pass an unrooted |left| to js_ConcatStrings. |left| in this case is a flat string, so we set |ldep = left| and realloc its buffer. Then, we call js_NewString to create a new string to return. Because of |gczeal(2)|, we GC the string pointed to by |left| and |ldep|. Now, js_NewGCThing runs and returns the first thing on its freelist, which happens to be the string formerly known as |left| and still known as |ldep|. Here's the problem:

(gdb) p str
$53 = (JSString *) 0x41c598
(gdb) p ldep
$54 = (JSString *) 0x41c598

The |JSPREFIX_INIT(ldep, str, ln)| on line 180 in jsstr.c is now setting both |str| and |ldep| to look like dependent strings.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible fix

This prevents |ldep| from being collected so that |str| cannot alias it.

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> Created an attachment (id=298132) [details]
> Possible fix
> 
> This prevents |ldep| from being collected so that |str| cannot alias it.

I do not think this is a good fix. There is no GC hazard with js_ConcatString unless one supplies an unrooted left or right arguments. Which points to FoldXMLConstants, that concatenates strings without rooting them (which I should have seen long time ago).

That is, FoldXMLConstants does not root accum assuming that weak rooting works. But it does not as TOK_XMLCOMMENT or TOK_XMLCDATA will displace accum from the newborn root and than later call js_ConcatStrings(cx, accuum, str) with the unrooted accum.

Comment 9

[Brendan Eich [:brendan]]

Right, js_ConcatStrings should be fast. Its callers are obligated to root inputs.

/be

Comment 10

[Brendan Eich [:brendan]]

Blake's in town, this may get fixed sooner.

/be

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

I don't know where to target this to... Guessing.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better fix

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 306789 [details] [diff] [review]
Better fix

This patch fixes a GC hazard.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306789 [details] [diff] [review]
Better fix

We'll take this after beta4.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306789 [details] [diff] [review]
Better fix

a1.9=beltzner

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.

Comment 17

[Bob Clary [:bc] (inactive)]

Attachment: e4x/extensions/regress-393874.js

Comment 18

[Bob Clary [:bc] (inactive)]

v 1.9.0

Comment 19

[Bob Clary [:bc] (inactive)]

when this bug is opened, the test should be checked in.