1267364 - topcrash in js::Shape::search<T> starting in 2016-04-25 builds, EXCEPTION_ACCESS_VIOLATION_READ crashes at 0xd or 0x15

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Baron :dbaron:]

This bug was filed from the Socorro interface and is 
report bp-4692e210-2348-488e-92ab-e76452160425.
=============================================================

There's a new topcrash in today's builds, at very large numbers (the point where we might want to respin nightly with a backout).  In most cases, the URL is  https://e.mail.ru/messages/inbox/ .

The crash address is always 0xd on x86 and 0x15 on amd64.

(There were previously a small number of crashes with this crash signature, but they weren't EXCEPTION_ACCESS_VIOLATION_READ crashes with those crash addresses.)

Comment 1

[David Baron :dbaron:]

Regression window is:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ab8a76ac7b34&tochange=1c6385ae1fe7

Comment 2

[David Baron :dbaron:]

Frame #3 of the stack goes through code added in https://hg.mozilla.org/mozilla-central/rev/8b08faebf408 which is in that range

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Arai-san requests for this to be s-s over IRC.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Check isNative every time in GetStringDataProperty.

GetElemBaseForLambda checked elemBase is native, but it can be swapped before GetStringDataProperty, by JSObject::swap, so added isNative check to GetStringDataProperty.

Comment 5

[Tooru Fujisawa [:arai]]

Sorry, forgot to store the fix for the comment in GetStringDataProperty

> +        // The object is already checked to be native in GetElemBaseForLambda,
> +        // but it can be swapped to the other class that is non-native.

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 8745047 [details] [diff] [review]
Check isNative every time in GetStringDataProperty.

Review of attachment 8745047 [details] [diff] [review]:
-----------------------------------------------------------------

Can you also assert on:
https://hg.mozilla.org/mozilla-central/rev/8b08faebf408#l4.79

That the return value of GetStringDataProperty is an string?

::: js/src/builtin/RegExp.cpp
@@ +1712,5 @@
>      MOZ_ASSERT(args.length() == 2);
>  
> +    RootedObject obj(cx, &args[0].toObject());
> +    if (!obj->isNative()) {
> +        // The object is already chekced to be native, but it can be swapped.

Next to the changes you mentioned in the above comment. Please also add:
// Return undefined to mark failure to get the property.

::: js/src/jsarray.cpp
@@ +3211,5 @@
>  bool
>  js::ArrayConstructor(JSContext* cx, unsigned argc, Value* vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    return ArrayConstructorImpl(cx, args, /* isConstructor = */ true);

Not part of this patch.

@@ +3221,5 @@
>      CallArgs args = CallArgsFromVp(argc, vp);
>      MOZ_ASSERT(!args.isConstructing());
>      MOZ_ASSERT(args.length() == 1);
>      MOZ_ASSERT(args[0].isNumber());
> +    return ArrayConstructorImpl(cx, args, /* isConstructor = */ false);

Ditto.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Check isNative every time in GetStringDataProperty. r=h4writer

Based on the discussion on IRC, changed the testcase to use really non-native object: Proxy, unboxed object, and typed object

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 8745088 [details] [diff] [review]
Check isNative every time in GetStringDataProperty. r=h4writer

Review of attachment 8745088 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome testcase!

Comment 9

[Tooru Fujisawa [:arai]]

thank you again :)

will land this after try run, hopefully in 2 hours.

Comment 10

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09aaae5469188c49e28668f407853a25e242aa58
Bug 1267364 - Check isNative every time in GetStringDataProperty. r=h4writer

Comment 11

[David Baron :dbaron:]

[Tracking Requested - why for this release]:

This needs to land on Aurora as well, since there was an uplift today.  (Or the other patch should be backed out on Aurora.)

And it should land *before* Aurora updates are turned on.  It is by far the #1 topcrash on nightly.

Comment 12

[Tooru Fujisawa [:arai]]

Comment on attachment 8745088 [details] [diff] [review]
Check isNative every time in GetStringDataProperty. r=h4writer

asking approval in advance in order to land this quickly.

Approval Request Comment
[Feature/regressing bug #]: bug 1264264
[User impact if declined]: topcrash
[Describe test coverage new/current, TreeHerder]: tested in mozilla-inbound
[Risks and why]: Low, this adds one more fallback path for the unhandled case, the fallback path was already tested.
[String/UUID change made/needed]: None

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8745088 [details] [diff] [review]
Check isNative every time in GetStringDataProperty. r=h4writer

Taking it before we enable updates.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09aaae546918

Comment 15

[Carsten Book [:Tomcat]]

has problems applying to aurora:

merging js/src/builtin/RegExp.cpp                                                                                                                                                                           
warning: conflicts while merging js/src/builtin/RegExp.cpp! (edit, then use 'hg resolve --mark')                                                                                                            
abort: unresolved conflicts, can't continue                                                                                                                                                                 
(use hg resolve and hg graft --continue)
Tomcats-MacBook-Pro-2:mozilla-central Tomcat$

Comment 16

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #15)
> has problems applying to aurora:
> 

oh wait, my bad.

Pushed now https://hg.mozilla.org/releases/mozilla-aurora/rev/cbeae0c4410b

Comment 17

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Tracking in case this reopens.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'js::Shape::search<T>':
 - nightly (version 50): 8 crashes from 2016-06-06.
 - aurora  (version 49): 30 crashes from 2016-06-07.
 - beta    (version 48): 108 crashes from 2016-06-06.
 - release (version 47): 650 crashes from 2016-05-31.
 - esr     (version 45): 0 crash from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          2          1          0          0          2          2          1
 - aurora           7          0          3          4          3         10          1
 - beta            11         18          6         30         20         15          5
 - release        192        116        120         78         37         32          6
 - esr              0          0          0          0          0          0          0

Affected platforms: Windows, Linux