Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Description • 8 years ago

The following testcase crashes on mozilla-central revision ab0044bfa1df (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --ion-offthread-compile=off --no-baseline --no-ion):

// Adapted from randomly chosen test: js/src/jit-test/tests/basic/bug1236476.js
oomTest(function() {
    offThreadCompileScript("");
});
// jsfunfuzz-generated
"".match();

Backtrace:

For detailed crash information, see attachment.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 2 • 8 years ago

Not sure if this OOM_VERBOSE=1 stack is relevant.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 3 • 8 years ago

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/b5b06959919a
user:        Tooru Fujisawa
date:        Sat Sep 05 21:55:06 2015 +0900
summary:     Bug 887016 - Part 9: Implement RegExp.prototype[@@match] and call it from String.prototype.match. r=till

arai-san, is bug 887016 a likely regressor?

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 4 • 8 years ago

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   js-dbg-64-dm-clang-darwin-ab0044bfa1df	0x0000000100022c8d JS::Value::toObject() const + 189 (Value.h:1281)
1   js-dbg-64-dm-clang-darwin-ab0044bfa1df	0x000000010004072c js::RegExpPrototypeOptimizable(JSContext*, unsigned int, JS::Value*) + 76 (Value.h:1765)
2   js-dbg-64-dm-clang-darwin-ab0044bfa1df	0x00000001007e3e6e js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 222 (jscntxtinlines.h:236)
3   js-dbg-64-dm-clang-darwin-ab0044bfa1df	0x00000001007d43fe js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 702 (Interpreter.cpp:468)
4   js-dbg-64-dm-clang-darwin-ab0044bfa1df	0x00000001007c9342 Interpret(JSContext*, js::RunState&) + 48322 (Interpreter.cpp:2831)
5   js-dbg-64-dm-clang-darwin-ab0044bfa1df	0x00000001007bd5b7 js::RunScript(JSContext*, js::RunState&) + 519 (Interpreter.cpp:426)
/snip

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 5 • 8 years ago

arai asked to mark it as s-s

	Tooru Fujisawa [:arai] Assignee
	Comment 6 • 8 years ago

Thanks :)

This is similar case as `initStringClass`, that happens in bug 1263558.
  https://hg.mozilla.org/integration/mozilla-inbound/rev/b1e8dbf2f4c92666991b0a026dfbc8fa0fa26826

This time it happens in `GlobalObject::resolveConstructor`.

https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/js/src/vm/GlobalObject.cpp#224
>     global->setConstructor(key, ObjectValue(*ctor));
>     global->setConstructorPropertySlot(key, ObjectValue(*ctor));
> 
>     // Define any specified functions and properties, unless we're a dependent
>     // standard class (in which case they live on the prototype), or we're
>     // operating on the self-hosting global, in which case we don't want any
>     // functions and properties on the builtins and their prototypes.
>     if (!StandardClassIsDependent(key) && !cx->runtime()->isSelfHostingGlobal(global)) {
>         if (const JSFunctionSpec* funs = clasp->specPrototypeFunctions()) {
>             if (!JS_DefineFunctions(cx, proto, funs))
>                 return false;
>         }

There, `GlobalObject::setConstructor` is called but the code after that may return with failure on OOM.  In that case, the constructor's `prototype` property is left `undefined`, and `RegExpProto` in the following code becomes `undefined`.

https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/js/src/builtin/String.js#16
> function IsStringMatchOptimizable() {
>     var RegExpProto = GetBuiltinPrototype("RegExp");
>     // If RegExpPrototypeOptimizable succeeds, `exec` and `@@match` are
>     // guaranteed to be data properties.
>     return RegExpPrototypeOptimizable(RegExpProto) &&
>            RegExpProto.exec === RegExp_prototype_Exec &&
>            RegExpProto[std_match] === RegExpMatch;
> }


Then, this time, we cannot move `setConstructor` after defining properties.
In JSProto_Function's case, `Function` constructor is referred from `JS_DefineFunctions`, so we should call `setConstructor` *before* it to make it accessible.

Thus, we should reset constructor slot when any of operations *after* `setConstructor` fails.

	Tooru Fujisawa [:arai] Assignee
	Comment 7 • 8 years ago

Changed all "return false" after `setConstructor` to "goto failedAfterSetConstructor", and in `failedAfterSetConstructor:` label, there `setConstructor` and `setConstructorPropertySlot` are called with `UndefinedValue()`, so that `resolveConstructor` will be called again on next time.

	Tooru Fujisawa [:arai] Assignee
	Comment 8 • 8 years ago

Also, there are some more cases that setConstructor and initBuiltinConstructor are called before defining properties.

	Till Schneidereit [:till]
	Comment 9 • 8 years ago

Comment on attachment 8746050 [details] [diff] [review]
Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor.

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

Hmm, do we need to use goto here? Can't we not move all the code that contains the gotos into a new function and do "if (!foo(..)) {do the reset stuff; return false;} else {return true;}?

	Tooru Fujisawa [:arai] Assignee
	Comment 10 • 8 years ago

Thank you for prompt response!

Changed it to a function.  Do you know any nice shorter name for the function?

	Till Schneidereit [:till]
	Comment 11 • 8 years ago

Comment on attachment 8746079 [details] [diff] [review]
Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor.

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

Thanks, that looks much better. The name is not fantastic, but I can't think of anything better there, so let's keep it.

	Till Schneidereit [:till]
	Comment 12 • 8 years ago

Comment on attachment 8746056 [details] [diff] [review]
Part 2: Call setConstructor and initBuiltinConstructor after defining properties in all init function.

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

r=me

	Tooru Fujisawa [:arai] Assignee
	Comment 13 • 8 years ago

Comment on attachment 8746079 [details] [diff] [review]
Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor.

Thank you for reviewing :D

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
This particular case (the testcase in comment #0) is not so easy, as it's just a null dereference, and it also needs testing functions that are not exposed to web content.
For underlying issue, I think everything else would also be a null dereference too, but I'm not sure how widely the issue affects.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The testcase describes how to cause crash, but with testing functions.

> Which older supported branches are affected by this flaw?
This particular flaw affects mozilla49 (nightly) and mozilla48 (aurora).
The underlying issue affects all branches.

> If not all supported branches, which bug introduced the flaw?
bug 887016 (mozilla48)

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Not yet, but it should be easily created.
Also, whole patches related to bug 887016 will be backed out from aurora in bug 1265307 anyway.

> How likely is this patch to cause regressions; how much testing does it need?
Less likely.  Automated tests should be able to catch anything happens.

	Al Billings [:abillings - ex-MoCo]
	Comment 14 • 8 years ago

If this is just a null dereference, why are you asking for sec-approval?

This bug needs a security rating but null dereferences are not normally sec-high or sec-critical, which are the only rating that require security approvals. I assume that this is a sec-low if it really is just a null dereference.

https://wiki.mozilla.org/Security/Bug_Approval_Process

	Tooru Fujisawa [:arai] Assignee
	Comment 15 • 8 years ago

Comment on attachment 8746079 [details] [diff] [review]
Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor.

Thanks!
clearing sec-approval.  will land them shortly.

	Tooru Fujisawa [:arai] Assignee
	Comment 16 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d31171af48e4fe4e0e35e6e96b4c76504a7ee091
Bug 1268034 - Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/05c1151be7a7821136263bb46d887bf8e99ce64d
Bug 1268034 - Part 2: Call setConstructor and initBuiltinConstructor after defining properties in all init function. r=till

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 8 years ago

https://hg.mozilla.org/mozilla-central/rev/d31171af48e4
https://hg.mozilla.org/mozilla-central/rev/05c1151be7a7

	Fuzzing Team
	Comment 18 • 8 years ago

JSBugMon: This bug has been automatically verified fixed.

	Tooru Fujisawa [:arai] Assignee
	Comment 19 • 8 years ago

Comment on attachment 8746079 [details] [diff] [review]
Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor.

same patches are applicable to mozilla-aurora

Approval Request Comment
> [Feature/regressing bug #]
bug 887016 exposed the issue, but underlying issue exists from old branches.

> [User impact if declined]
Unnecessary crash when hitting potentially-recoverable OOM, by opening certain web page.

> [Describe test coverage new/current, TreeHerder]
Tested in mozilla-central automated test.

> [Risks and why]
Low.  Added recover code path for OOM.

> [String/UUID change made/needed]
None

	Tooru Fujisawa [:arai] Assignee
	Comment 20 • 8 years ago

Comment on attachment 8746079 [details] [diff] [review]
Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor.

withdrawing approval, due to bug 1269074.

	Tooru Fujisawa [:arai] Assignee
	Comment 21 • 8 years ago

So, we should do some more steps to rollback *all* operations done in GlobalObject::resolveConstructor when it fails.

	Tooru Fujisawa [:arai] Assignee
	Comment 22 • 8 years ago

at least, the following step should be rolled back, or skipped on the 2nd time

>    if (clasp->specShouldDefineConstructor()) {
>        if (!global->addDataProperty(cx, id, constructorPropertySlot(key), 0))
>            return false;
>    }

Also, the following should also be rolled back, as the prototype properties are not yet initialized.

>        global->setPrototype(key, ObjectValue(*proto));

	Tooru Fujisawa [:arai] Assignee
	Comment 23 • 8 years ago

and now I'm wondering if it's really recoverable.
Could it be possible that each hook (specCreateConstructorHook, specFinishInitHook) do un-recoverable operation?

	Tooru Fujisawa [:arai] Assignee
	Comment 24 • 8 years ago

for example, FinishObjectClassInit defines property on global object.
  https://dxr.mozilla.org/mozilla-central/rev/8c3fd523d75bd30f691ca2d6cfdad18d576392a1/js/src/builtin/Object.cpp#1194

this specific case could be ignored tho, I'm not sure if other (and future) cases are also recoverable.

should we reduce the target of recover operation?
or perhaps, should we throw away the assumption that constructor/prototype are initialized properly, especially even if the property is non-configurable? (I hope not...)


till, can I have your opinion for comment #21-#23, and bug 1269074's case?

	Fuzzing Team
	Comment 25 • 8 years ago

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 2b7c421063ad).

	Tooru Fujisawa [:arai] Assignee
	Comment 26 • 8 years ago

So, the issue initially hit with this not-properly-initialized constructor object was bug 1263558, there String.prototype was undefined.
and this bug is that RegExp.prototype was undefined.

we didn't check them because of the following assumption:

  "prototype" property should always be a prototype object for each constructor, because "prototype" property is non-configurable data property

But if constructor/prototype initialization can fail with OOM and can leave something partially-initialized, those assumption breaks.  This is also true for all other non-configurable properties if any (will check the spec).

Now we're going to rollback the initialization to make those partially-initialized object not-accessible from other code, and then perform the initialization again on the next time when the constructor/prototype is accessed.

So, those initialization operation should be able to be performed more than once, until the initialization completes without any error.

I'm not sure if that rule can be maintained...

	Tooru Fujisawa [:arai] Assignee
	Comment 27 • 8 years ago

In addition to "prototype" property, the following properties are non-configurable.

  Function.prototype[@@hasInstance]
  Symbol.hasInstance
  Symbol.isConcatSpreadable
  Symbol.iterator
  Symbol.match
  Symbol.prototype
  Symbol.replace
  Symbol.search
  Symbol.species
  Symbol.split
  Symbol.toPrimitive
  Symbol.toStringTag
  Symbol.unscopables
  Number.EPSILON
  Number.MAX_SAFE_INTEGER
  Number.MAX_VALUE
  Number.MIN_SAFE_INTEGER
  Number.MIN_VALUE
  Number.NaN
  Number.NEGATIVE_INFINITY
  Number.POSITIVE_INFINITY
  Math.E
  Math.LN10
  Math.LN2
  Math.LOG10E
  Math.LOG2E
  Math.PI
  Math.SQRT1_2
  Math.SQRT2
  %TypedArray%.BYTES_PER_ELEMENT
  %TypedArray%.prototype.BYTES_PER_ELEMENT

and Function and TypedArray uses InitViaClassSpec.

	Tooru Fujisawa [:arai] Assignee
	Comment 28 • 8 years ago

In case we should go with current approach, here's the patch that removes the constructor property from global object, so that the property is not in the global object's shape on the next time.

NativeDeleteProperty is fallible, and in failure case we cannot recover anymore, added MOZ_CRASH there but I'm not sure what's the best way there...

	Tooru Fujisawa [:arai] Assignee
	Comment 29 • 8 years ago

and one more patch to reset prototype slot of global object on failure case.

	Till Schneidereit [:till]
	Comment 30 • 8 years ago

Comment on attachment 8747412 [details] [diff] [review]
(WIP) Part 3: Delete global property when it fails to initialize constructor.

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

r=me

	Till Schneidereit [:till]
	Comment 31 • 8 years ago

Comment on attachment 8747413 [details] [diff] [review]
Part 4: Reset prototype slot of GlobalObject to undefined when it fails to initialize prototype.

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

r=me

	Tooru Fujisawa [:arai] Assignee
	Comment 32 • 8 years ago

Thank you for reviewing :)

Now checking each hook.
so far, CreateFunctionPrototype cannot be executed twice...

https://dxr.mozilla.org/mozilla-central/rev/1461a4071341c282afcf7b72e33036412d2251d4/js/src/jsfun.cpp#832
> static JSObject*
> CreateFunctionPrototype(JSContext* cx, JSProtoKey key)
> {
>     Rooted<GlobalObject*> self(cx, cx->global());
> ...
>     self->setThrowTypeError(throwTypeError);

https://dxr.mozilla.org/mozilla-central/rev/1461a4071341c282afcf7b72e33036412d2251d4/js/src/vm/GlobalObject.h#151
>     void setThrowTypeError(JSFunction* fun) {
>         MOZ_ASSERT(getSlotRef(THROWTYPEERROR).isUndefined());
>         setSlot(THROWTYPEERROR, ObjectValue(*fun));
>     }

The assertion fails.


This could happen in the following case:
  1. resolve Function constructor
  2. call createPrototype hook (CreateFunctionPrototype)
  3. OOM happens while defining prototype property
  4. tries to rollback, reset constructor slot (Part 1)

  5. resolve Function constructor because constructor slot is undefined
  6. call createPrototype hook (CreateFunctionPrototype) again


So we may need extra function to rollback each hook, or detect the 2nd execution inside each hook.

	Tooru Fujisawa [:arai] Assignee
	Comment 33 • 8 years ago

Also TypedArray's finishClassInit cannot be executed twice.

>     static bool
>     finishClassInit(JSContext* cx, HandleObject ctor, HandleObject proto)
>     {
> ....
>         cx->global()->setCreateArrayFromBuffer<NativeType>(fun);
>         return true;
>     }

>     void setCreateArrayFromBufferHelper(uint32_t slot, Handle<JSFunction*> fun) {
>         MOZ_ASSERT(getSlotRef(slot).isUndefined());
>         setSlot(slot, ObjectValue(*fun));
>     }

the assertion fails.


Object's FinishObjectClassInit seems to be also problematic.

> static bool
> FinishObjectClassInit(JSContext* cx, JS::HandleObject ctor, JS::HandleObject proto)
> {
> ...
>     global->setOriginalEval(evalobj);
> ...
>     if (global->shouldSplicePrototype(cx)) {
>         if (!global->splicePrototype(cx, global->getClass(), tagged))
>             return false;
>     }


>     void setOriginalEval(JSObject* evalobj) {
>         MOZ_ASSERT(getSlotRef(EVAL).isUndefined());
>         setSlot(EVAL, ObjectValue(*evalobj));
>     }

the assertion in setOriginalEval fails.


> bool
> JSObject::shouldSplicePrototype(JSContext* cx)
> {
>     /*
>      * During bootstrapping, if inference is enabled we need to make sure not
>      * to splice a new prototype in for Function.prototype or the global
>      * object if their __proto__ had previously been set to null, as this
>      * will change the prototype for all other objects with the same type.
>      */
>     if (getProto() != nullptr)
>         return false;
>     return isSingleton();
> }

I suppose `getProto() != nullptr` is different on 1st and 2nd execution.

	Tooru Fujisawa [:arai] Assignee
	Comment 34 • 8 years ago

now I'm thinking another way.

Can't we invalidate the global once constructor/prototype initialization fails with OOM?
like, setting yet another value to the constructor/prototype slot (maybe, a magic value?), and detect it and throw InternalError every time the script (or the internal) tries to access it.

So that we won't use the partially-initialized constructor/prototype anywhere, and we won't execute resolveConstructor twice.
of course the global cannot execute any of script from there, but it happens only when it hit OOM.
I feel it's safer than trying to recover from OOM.
(of course we have to make sure that it won't fall into infinite loop of throwing error tho)

	Till Schneidereit [:till]
	Comment 35 • 8 years ago

Bah, very annoying. I'm not sure the proposed solution really works. Perhaps at this point we should prevent all script execution in the affected global? I think we have the infrastructure for that.

Needinfo'ing jorendorff because I can't think of a good solution here that isn't somewhat radical.

	Tooru Fujisawa [:arai] Assignee
	Comment 36 • 8 years ago

WIP patches for the comment #34 idea, based on backing out Part 1.

First, Part 3 adds GlobalObject::overrideConstructor to distinguish between "newly setting constructor" and "overriding existing constructor".

	Tooru Fujisawa [:arai] Assignee
	Comment 37 • 8 years ago

Then, Part 4 adds new magic value to mark constructor/prototype slot that initialization is failed before.

	Tooru Fujisawa [:arai] Assignee
	Comment 38 • 8 years ago

forgot to refresh the patch

	Jason Orendorff [:jorendorff]
	Comment 39 • 8 years ago

The list of non-configurable properties does not seem like a problem to me. Math.PI, for example - what exactly can go wrong there?

The problem with the GlobalObject's prototypes and constructors is serious. If possible, we should always create the prototype-constructor pair and fully populate it before storing those objects in the GlobalObject. The property should be defined, and the reserved slots should be populated, only after everything else has succeeded. That way there is nothing to roll back on failure.

Arai, if I fix the %%ObjectPrototype%% and %%FunctionPrototype%% initialization, can the rest be fixed as described above?

	Tooru Fujisawa [:arai] Assignee
	Comment 40 • 8 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #39)
> The list of non-configurable properties does not seem like a problem to me.
> Math.PI, for example - what exactly can go wrong there?

just wanted to list up possibly affected properties.  if they're not problem, we just have to worry about 'prototype' property :)

Indeed, most of them won't be a problem, as those are really constants, and self-hosted code doesn't use those properties but global variables or macro like std_iterator or MAX_*.


> The problem with the GlobalObject's prototypes and constructors is serious.
> If possible, we should always create the prototype-constructor pair and
> fully populate it before storing those objects in the GlobalObject. The
> property should be defined, and the reserved slots should be populated, only
> after everything else has succeeded. That way there is nothing to roll back
> on failure.

Yeah, if it's possible, it should be much better solution than rollback or invalidate.


> Arai, if I fix the %%ObjectPrototype%% and %%FunctionPrototype%%
> initialization, can the rest be fixed as described above?

Will investigate it shortly.
Thanks!

	Tooru Fujisawa [:arai] Assignee
	Comment 41 • 8 years ago

For other cases than Function/Object, the issue is that the number of fallible operations that modifies global object, and we may have to perform small-rollback on failure case, or skip them on 2nd exection.


Here's the list of operations that modifies global object, done in resolveConstructor:
  fallible
    global->addDataProperty(cx, id, constructorPropertySlot(key), 0)
    finishInit(cx, ctor, proto)

  infallible
    global->setConstructor(key, ObjectValue(*ctor));
    global->setConstructorPropertySlot(key, ObjectValue(*ctor));
    global->setPrototype(key, ObjectValue(*proto));
    AddTypePropertyId(cx, global, id, ObjectValue(*ctor));

finishInit may modify global object (TypedArray's and Object's case, see comment #32), and there could be more than 1 operations inside.

for TypedArray's case, setCreateArrayFromBuffer itself is infallible, but other operations there (DefineProperty, NewNativeFunction) are fallible.  Anyway, as long as there are fallible operations inside finishInit, there is no good ordering between finishInit and addDataProperty, that can avoid rollback or skip.

So, we should do either:
  a) rollback addDataProperty when finishInit fails
  b) do not perform addDataProperty on 2nd execution (but assert that it has exactly same property)


Also, if finishInit itself may contain more than 1 fallible operations that modifies global object, finishInit should handle the failure case.

	Tooru Fujisawa [:arai] Assignee
	Comment 42 • 8 years ago

Here's WIP patch that modifies global slots/properties after populating/linking constructor/prototypes, for non-Function/Object.
It passes jstests and jittest, so I think everything else than Function/Object can be fixed with this way.

	Jason Orendorff [:jorendorff]
	Comment 43 • 8 years ago

I think the part of typed arrays' finishClassInit hook that modifies the global object should be removed.

Instead, at the point in fromBufferWithProto where it calls cx->global()->createArrayFromBuffer<NativeType>, it should create that function on demand, and cache it on success.

	Jason Orendorff [:jorendorff]
	Comment 44 • 8 years ago

finishClassInit might be a bad design to begin with. The way to do a complex fallible operation is to do as much work as possible in isolation, then make it public ("commit" it) in a single step. (cf. transactions)

So having a fallible hook that's called *after* class initialization is committed (by defining a global property) is questionable.

	Tooru Fujisawa [:arai] Assignee
	Comment 45 • 8 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #43)
> I think the part of typed arrays' finishClassInit hook that modifies the
> global object should be removed.
> 
> Instead, at the point in fromBufferWithProto where it calls
> cx->global()->createArrayFromBuffer<NativeType>, it should create that
> function on demand, and cache it on success.

Okay, will fix it first.


(In reply to Jason Orendorff [:jorendorff] from comment #44)
> finishClassInit might be a bad design to begin with. The way to do a complex
> fallible operation is to do as much work as possible in isolation, then make
> it public ("commit" it) in a single step. (cf. transactions)
> 
> So having a fallible hook that's called *after* class initialization is
> committed (by defining a global property) is questionable.

There are 2 kind of things done in finishInit hook,

  A) modifies constructor or prototype object
  B) modifies global object

and (A) won't be a problem, and it will make things easier,
also it can be done as a part of constructor/prototype initialization.

So, how about reducing the things can be done in finishInit hook only to (A) ?


Here's the list of things done in each prototype's finishInit hook.

  Object -- FinishObjectClassInit
    (B) defines global.eval
    (B) stores the original eval function to global (global->setOriginalEval)
    (B) install the intrinsics holder to global (GlobalObject::getIntrinsicsHolder)
    (B) splice global object's prototype (global->splicePrototype)

  Array -- array_proto_finish
    (A) defines Array.prototype[@@unscopables]

  Date -- FinishDateClassInit
    (A) defines Date.prototype.toUTCString
    (A) defines Date.prototype.toGMTString

  TypedArray -- _type##Array::finishClassInit
    (A) defines ctor.BYTES_PER_ELEMENT
    (A) defines ctor.prototype.BYTES_PER_ELEMENT
    (B) creates `CreateArrayFromBuffer` function and stores it to global (cx->global()->setCreateArrayFromBuffer)
        => move to other place

  SavedFrame -- SavedFrame::finishSavedFrameInit
    (A) stores NullValue to SavedFrame.prototype's SavedFrame::JSSLOT_SOURCE slot
    (A) freezes SavedFrame.prototype


Once TypedArray's case is fixed, Object's case is only the remaining issue, and I think it should be done in different place than finishInit hook, as most of them are not related to Object prototype itself, but just a dependencies.

	Tooru Fujisawa [:arai] Assignee
	Comment 46 • 8 years ago

Moved ArrayBufferObject::createTypedArrayFromBuffer function creation to dedicated function, that is called when invoking it.

`!getSlot(slot).isUndefined()` assertion in GlobalObjectGlobalObject:createArrayFromBufferHelper is removed as we need to access it to check if it's cached or not.

	Tooru Fujisawa [:arai] Assignee
	Comment 47 • 8 years ago

now finishInit hook doesn't modify global for non-Object case.
moved it to initialization.

	Jason Orendorff [:jorendorff]
	Comment 48 • 8 years ago

Thank you for the list in comment 45. That's great work.

	Jason Orendorff [:jorendorff]
	Comment 49 • 8 years ago

Comment on attachment 8749363 [details] [diff] [review]
Part 3: Create CreateArrayFromBuffer function on demand.

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

r=me. Thanks!

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 50 • 8 years ago

Arai-san, what's next here?

	Tooru Fujisawa [:arai] Assignee
	Comment 51 • 8 years ago

Thank you for reminder :)

So, the already-confirmed issues (RegExp and String prototypes) can be fixed by, backing out Part 1, and landing Part 3 and Part 4 (it needs cleanup tho),
but it leaves the issue on Object and Function prototypes.

jorendorff, what's your plan about Object and Function prototypes, regarding comment #39 ?
can these be left for now?

	Jason Orendorff [:jorendorff]
	Comment 52 • 8 years ago

If you can land a fix for everything else, land it.

Fixing Object and Function startup is a lot more work. I have a partial patch, but no time to work on it.

	Tooru Fujisawa [:arai] Assignee
	Comment 53 • 8 years ago

Thanks :)

Reduced duplicated code.

This patch changes the order of initialization for non-Object/Function, and do operations that modifies global object after all other things.

	Jason Orendorff [:jorendorff]
	Comment 54 • 8 years ago

Comment on attachment 8753911 [details] [diff] [review]
Part 4: Delay modifying global constructor/prototype slots for classes other than Function and Object.

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

Great. Thanks.

::: js/src/vm/GlobalObject.cpp
@@ +266,5 @@
> +    if (!isObjectOrFunction) {
> +        // Any operations that modifies global object should be placed after
> +        // any other fallible operations.
> +
> +        // Fallible operations that modifies global object.

Grammar nit:
        // Fallible operation that modifies the global object.

@@ +272,5 @@
> +            if (!global->addDataProperty(cx, id, constructorPropertySlot(key), 0))
> +                return false;
> +        }
> +
> +        // Infallible operations that modifies global object.

// Infallible operations that modify the global object.

	Tooru Fujisawa [:arai] Assignee
	Comment 55 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7861d6b6c274db85036adc5496718d456e3a78c8
Backed out changeset d31171af48e4 (bug 1268034)

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c2a58d84d498bd8841f3bd88e6091ebcba58818
Bug 1268034 - Part 3: Create CreateArrayFromBuffer function on demand. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e198b4ccab26156c3dd6417b2ee08d53ab622dd
Bug 1268034 - Part 4: Delay modifying global constructor/prototype slots for classes other than Function and Object. r=jorendorff

	Ryan VanderMeulen [:RyanVM]
	Comment 56 • 8 years ago

https://hg.mozilla.org/mozilla-central/rev/7861d6b6c274
https://hg.mozilla.org/mozilla-central/rev/9c2a58d84d49
https://hg.mozilla.org/mozilla-central/rev/1e198b4ccab2

	Tooru Fujisawa [:arai] Assignee
	Comment 57 • 8 years ago

The testcase in comment #0 is specific to bug 887016.

The underlying issue fixed by patches in this bug affects all branches tho, that won't be critical for other branches than m-c.

	Fuzzing Team
	Comment 58 • 8 years ago

JSBugMon: This bug has been automatically verified fixed.