584579 - JM: mochitest-plain crash on dom/tests/mochitest/ajax/prototype/test_Prototype.html

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

We crash an a match function called from str_IndexOf. It looks like the argument is a String object that is not getting unwrapped correctly, but instead gets cast directly to a string. Or something.

Comment 1

[David Mandelin [:dmandelin]]

Attachment: Reduced test case (I think)

I think we might be going wrong in the callprop in g().

Comment 2

[David Mandelin [:dmandelin]]

Attachment: Patch

The cause was that jsop_binary tries to optimize storing a string-tagged result if either operand is a string. But if one operand is a string object, then the result is a string object, so the condition must be strengthened to "one operand is a string, the other is not an object".

Comment 3

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/projects/jaegermonkey/rev/722420b0c4c8

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> 
> But if one operand is a string object, then the
> result is a string object

Are you sure about that?

  js> typeof("1" + new String('abc'))
  "string"
  js> typeof(new String('abc') + "1") 
  "string"

Am I missing something?

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #4)

Good catch. Something more sinister is at play here. stubs::Add() returns a JSVAL_TYPE_STRING in both cases. Backing out this patch locally, it flows into ic::GetProp, which sees a string object.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

There were two bugs here. The first is that CALLPROP on a statically known string didn't sync the right amount of values when wrapping a primitive. The other is that after wrapping a primitive, it still tracked the value as a string.

The original fix masked the problem by eliminating cases where callprop_str could be used.

http://hg.mozilla.org/projects/jaegermonkey/rev/225495aeb3d5

Comment 7

[Brendan Eich [:brendan]]

Why wrap a string constant with a String object to call a method on it? The implicit conversion should be eliminated if the method is one of the well-known natives on String.prototype. Sorry if that is ruled out here and a possibly user-defined method is being called.

Even if not a well-known native, if the function uses "use strict"; then it'll need to get a primitive string as |this|.

/be

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #7)

Yeah, we inline a primitive-this test and take an out-of-line path to wrap. This path is generic, we don't guess the method at compile-time (yet - seems like future work could be done there, if it's worth it.)