1320408 - Change method that does GC on |this| to static method with Handle parameter.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Tooru Fujisawa [:arai]]

Some methods do |Rooted<SOMETHING> self(cx, this)| to access correct |this| object after GC, but it's dangerous.
such method should be changed to static method that takes |Handle<SOMETHING>| parameter.

Comment 1

[Tooru Fujisawa [:arai]]

here's the list that explicitly add root to this
  https://dxr.mozilla.org/mozilla-central/search?q=self(cx%2C%20this)&case=true&=mozilla-central

I'm about to change them to static method, and also change all other method that takes JSContext* to either static method or some special name (infallibleSOMETHING).

Comment 2

[Tooru Fujisawa [:arai]]

so far, here's patch series that changes methods that GC to static method, or just remove JSContext parameter if not necessary.  it's already long so I'd like to  go with those patches first.

https://hg.mozilla.org/try/pushloghtml?changeset=fe2ca06d270eb4f68db9392af1df131393dd0833

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Change JSFunction::getLength and JSFunction::getUnresolvedLength to static method.

I'd like to change JSFunction methods first to fix Function#name (this patch is not required for it tho, it's nice to have before modifying those methods)

JSFunction::getLength does |JS::RootedFunction self(cx, this);| at the top of the function and uses |self| for later access, but |this| is still available and if one forgets to write |self->|, it compiles without any error.
(actually I hit that while creating bug 1320388 patch :P

I believe we shouldn't use instance method if |this| can be GCed, but we should use static method that takes Handle instead, so that there's no dangerous |this|.

This patch changes JSFunction::getLength and JSFunction::getUnresolvedLength to static method.

remaining patches do almost same thing for several methods.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2632a7acc8e03dd16d1b45df548c217e62cd80b0&selectedJob=31916820

Comment 4

[Tooru Fujisawa [:arai]]

here's entire patch list https://hg.mozilla.org/try/pushloghtml?changeset=0845b547f37e

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Change JSFunction::getOrCreateScript to static method.

Same for JSFunction::getOrCreateScript.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Remove JSContext* parameter from JSFunction::getBoundFunctionArgument.

while checking, noticed that JSFunction::getBoundFunctionArgument doesn't need JSContext*.
so just removed the parameter.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8814646 [details] [diff] [review]
Part 1: Change JSFunction::getLength and JSFunction::getUnresolvedLength to static method.

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

::: js/src/vm/SelfHosting.cpp
@@ +461,5 @@
>      // Try to avoid invoking the resolve hook.
>      if (targetObj->is<JSFunction>() && !targetObj->as<JSFunction>().hasResolvedLength()) {
>          RootedValue targetLength(cx);
> +        RootedFunction targetFun(cx, &targetObj->as<JSFunction>());
> +        if (!JSFunction::getUnresolvedLength(cx, targetFun, &targetLength))

Nit: as this code is a little perf-sensitive, we can avoid the extra root like this:

  HandleFunction targetFun = targetObj.as<JSFunction>();

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8814647 [details] [diff] [review]
Part 2: Change JSFunction::getOrCreateScript to static method.

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

Nice that most callers already had a HandleFunction.

::: js/src/jsobj.cpp
@@ +951,5 @@
>      }
>  
>      if (res) {
> +        RootedFunction fun(cx, &callee->as<JSFunction>());
> +        JSScript* script = JSFunction::getOrCreateScript(cx, fun);

Here we can also avoid the rooted:

  JSScript* script = JSFunction::getOrCreateScript(cx, callee.as<JSFunction>());

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8814648 [details] [diff] [review]
Part 3: Remove JSContext* parameter from JSFunction::getBoundFunctionArgument.

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

Nice find.

Comment 10

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)
I'll land patches incrementally.

Comment 11

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20c75f210e9a2abb0d6a932adb69b88a27579846
Bug 1320408 - Part 1: Change JSFunction::getLength and JSFunction::getUnresolvedLength to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b4e659892cee5ac0d89931f79bdf367807bf3c2
Bug 1320408 - Part 2: Change JSFunction::getOrCreateScript to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/b509be16fc198e36e326f7fead273c273e3cd207
Bug 1320408 - Part 3: Remove JSContext* parameter from JSFunction::getBoundFunctionArgument. r=jandem

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/20c75f210e9a
https://hg.mozilla.org/mozilla-central/rev/6b4e659892ce
https://hg.mozilla.org/mozilla-central/rev/b509be16fc19

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Change JSObject::getGroup to static method.

Changed JSObject::getGroup to static method.
and also changed some functions that calls JSObject::getGroup to static method.

|RootedObject self(cx, this)| added to JSObject::constructorDisplayAtom is removed in next part.

I have a question in TypeInference.cpp.
in js::EnsureTrackPropertyTypes, non-rooted |obj| variable is used across JSObject::getGroup call, and also it matches |obj->hasLazyGroup()|, is it safe?

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Change JSObject::constructorDisplayAtom to static method.

Also removed temporary fix in Part 4.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Change JSObject::splicePrototype to static method.

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Remove JSContext* parameter from JSObject::shouldSplicePrototype.

Just removed unused JSContext* parameter from shouldSplicePrototype.

Comment 17

[Jan de Mooij [:jandem]]

(In reply to Tooru Fujisawa [:arai] from comment #13)
> I have a question in TypeInference.cpp.
> in js::EnsureTrackPropertyTypes, non-rooted |obj| variable is used across
> JSObject::getGroup call, and also it matches |obj->hasLazyGroup()|, is it
> safe?

Yeah because it uses AutoEnterAnalysis and that has an AutoSupressGC. That's why the rooting analysis doesn't complain :)

Comment 18

[Jan de Mooij [:jandem]]

Comment on attachment 8821449 [details] [diff] [review]
Part 4: Change JSObject::getGroup to static method.

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

::: js/src/vm/TypeInference.cpp
@@ +1323,5 @@
>      if (obj->isSingleton()) {
>          AutoEnterAnalysis enter(cx);
>          if (obj->hasLazyGroup()) {
>              AutoEnterOOMUnsafeRegion oomUnsafe;
> +            RootedObject obj_(cx, obj);

Nit: maybe objRoot?

Comment 19

[Jan de Mooij [:jandem]]

Comment on attachment 8821451 [details] [diff] [review]
Part 6: Change JSObject::splicePrototype to static method.

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

Nice!

Comment 20

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d2ebd012bd504f744cfb638100a6429e7349b86
Bug 1320408 - Part 4: Change JSObject::getGroup to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d62e09d7c87f8464b00316de4402fea381453b9
Bug 1320408 - Part 5: Change JSObject::constructorDisplayAtom to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/d56269f92df741484eb92e08564185ae0df55c08
Bug 1320408 - Part 6: Change JSObject::splicePrototype to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/2caf84698f926b00b3f092694b76924e656ee867
Bug 1320408 - Part 7: Remove JSContext* parameter from JSObject::shouldSplicePrototype. r=jandem

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3d2ebd012bd5
https://hg.mozilla.org/mozilla-central/rev/1d62e09d7c87
https://hg.mozilla.org/mozilla-central/rev/d56269f92df7
https://hg.mozilla.org/mozilla-central/rev/2caf84698f92

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Change JSObject::reportNotConfigurable and JSObject::reportNotExtensible to static method.

just like JSObject::reportReadOnly, changed other report methods static.

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Part 9: Change JSObject::setFlags and depending methods to static method.

Changed JSObject::setFlags to static and removed self variable.
and also changed instance methods setSomething that calls JSObject::setFlags to static.

Comment 24

[Tooru Fujisawa [:arai]]

Attachment: Part 10: Change LazyScript::functionDelazifying to static method.

Changed LazyScript::functionDelazifying to static and removed self variable

Comment 25

[Tooru Fujisawa [:arai]]

Attachment: Part 11: Remove JSContext* parameter from JSScript::ensureNonLazyCanonicalFunction.

Just removed unused JSContext parameter from JSScript::ensureNonLazyCanonicalFunction.

Comment 26

[Tooru Fujisawa [:arai]]

Attachment: Part 12: Change JSScript::sourceData to static method.

This may be unnecessary change tho, changed JSScript::sourceData to static.
Feel free to reject ;)

Comment 27

[Tooru Fujisawa [:arai]]

Attachment: Part 13: Change DebugEnvironmentProxy::getMaybeSentinelValue to static method.

Changed DebugEnvironmentProxy::getMaybeSentinelValue to static and removed self variable

Comment 28

[Tooru Fujisawa [:arai]]

Attachment: Part 14: Change some GlobalObject methods to static method.

Changed GlobalObject::createBlankPrototype, GlobalObject::createBlankPrototypeInheriting, GlobalObject::getRegExpStatics, GlobalObject::getOrCreate*,
and removed self variables there.

Comment 29

[Jan de Mooij [:jandem]]

Comment on attachment 8824675 [details] [diff] [review]
Part 14: Change some GlobalObject methods to static method.

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

Great!

::: js/src/builtin/Reflect.cpp
@@ +267,5 @@
>  
>  JSObject*
>  js::InitReflect(JSContext* cx, HandleObject obj)
>  {
> +    Rooted<GlobalObject*> global(cx, &obj->as<GlobalObject>());

Nit: this can be Handle<GlobalObject*> global = obj.as<GlobalObject>();

Also a few other times (search for "->as<GlobalObject>")

::: js/src/jsexn.cpp
@@ +515,5 @@
>  ErrorObject::createProto(JSContext* cx, JSProtoKey key)
>  {
>      JSExnType type = ExnTypeFromProtoKey(key);
>  
>      if (type == JSEXN_ERR)

Nit: needs {} now

Comment 30

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/513d1e41b5940e9b2d6978ff25d6dab0f5579e53
Bug 1320408 - Part 8: Change JSObject::reportNotConfigurable and JSObject::reportNotExtensible to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/43ac5e5b71937a4cbad6dcc525be975a37f84a95
Bug 1320408 - Part 9: Change JSObject::setFlags and depending methods to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/81f6417e94c4bfc6812225a22d8599a09c5cd8ce
Bug 1320408 - Part 10: Change LazyScript::functionDelazifying to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8daa94965062c56622fdcc646e0d1d3442f382
Bug 1320408 - Part 11: Remove JSContext* parameter from JSScript::ensureNonLazyCanonicalFunction. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c611312df295461159e240077c566ea4877dd43
Bug 1320408 - Part 12: Change JSScript::sourceData to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/93ce2713f21dca17968d4080700a6eed22f75939
Bug 1320408 - Part 13: Change DebugEnvironmentProxy::getMaybeSentinelValue to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdafc05f51e8164e3a8923637f7248f7c1124066
Bug 1320408 - Part 14: Change some GlobalObject methods to static method. r=jandem

Comment 31

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/513d1e41b594
https://hg.mozilla.org/mozilla-central/rev/43ac5e5b7193
https://hg.mozilla.org/mozilla-central/rev/81f6417e94c4
https://hg.mozilla.org/mozilla-central/rev/6d8daa949650
https://hg.mozilla.org/mozilla-central/rev/1c611312df29
https://hg.mozilla.org/mozilla-central/rev/93ce2713f21d
https://hg.mozilla.org/mozilla-central/rev/bdafc05f51e8

Comment 32

[Tooru Fujisawa [:arai]]

Attachment: Part 15: Change NativeObject::addDataProperty to static method.

Removed `self` in NativeObject::addDataProperty.

Comment 33

[Tooru Fujisawa [:arai]]

Attachment: Part 16: Change NativeObject::removeProperty to static method.

Removed `self` in NativeObject::removeProperty.

Comment 34

[Tooru Fujisawa [:arai]]

Attachment: Part 17: Change NativeObject::clearFlag to static method.

Removed `self` in NativeObject::clearFlag.

Comment 35

[Tooru Fujisawa [:arai]]

Attachment: Part 18: Change StringObject::init to static method.

Removed `self` in StringObject::init.

Comment 36

[Tooru Fujisawa [:arai]]

Attachment: Part 19: Remove JSContext* parameter from ModuleObject::fixEnvironmentsAfterCompartmentMerge.

Removed unused `JSContext* cx` parameter from ModuleObject::fixEnvironmentsAfterCompartmentMerge.

Comment 37

[Tooru Fujisawa [:arai]]

Attachment: Part 20: Change PromiseObject::resolve and PromiseObject::reject to static method.

PromiseObject::resolve and PromiseObject::reject don't have `self`, but still referring `this`.
So changed them to static method to prevent rooting-related issue.

Comment 38

[Tooru Fujisawa [:arai]]

Attachment: Part 21: Change PromiseObject::onSettled to static method.

Removed `promise` variable that is actually `self`, from PromiseObject::onSettled.

Comment 39

[Jan de Mooij [:jandem]]

Comment on attachment 8831968 [details] [diff] [review]
Part 21: Change PromiseObject::onSettled to static method.

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

Great job, as always!

Comment 40

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42b3573b6996fca8278b396ff0ebfce1d49402b1
Bug 1320408 - Part 15: Change NativeObject::addDataProperty to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/c16497ad79a932392108ed7dc8df9c4b521d670f
Bug 1320408 - Part 16: Change NativeObject::removeProperty to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/2909aaf6bc2a5486f39263bdb1cb6ddd56c738cd
Bug 1320408 - Part 17: Change NativeObject::clearFlag to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/21048aea27933830eb8eb9c8f733a7a159384c66
Bug 1320408 - Part 18: Change StringObject::init to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4279aa647a30fb9d700b474845e933958f02087
Bug 1320408 - Part 19: Remove JSContext* parameter from ModuleObject::fixEnvironmentsAfterCompartmentMerge. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/906483bd4d5e04342e08254ec4c200d9425e6175
Bug 1320408 - Part 20: Change PromiseObject::resolve and PromiseObject::reject to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f346aaca83b47a62360bc52177c1f77efe3a909
Bug 1320408 - Part 21: Change PromiseObject::onSettled to static method. r=jandem

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/42b3573b6996
https://hg.mozilla.org/mozilla-central/rev/c16497ad79a9
https://hg.mozilla.org/mozilla-central/rev/2909aaf6bc2a
https://hg.mozilla.org/mozilla-central/rev/21048aea2793
https://hg.mozilla.org/mozilla-central/rev/f4279aa647a3
https://hg.mozilla.org/mozilla-central/rev/906483bd4d5e
https://hg.mozilla.org/mozilla-central/rev/9f346aaca83b

Comment 42

[Tooru Fujisawa [:arai]]

Attachment: Part 22: Remove JSContext* parameter from ProxyObject::renew and Wrapper::Renew.

Just removed unused parameter.

Comment 43

[Tooru Fujisawa [:arai]]

Attachment: Part 23: Change RegExpObject::{getShared,createShared,dumpBytecode} to static method.

Removed `self` from RegExpObject::createShared, and changed depending methos (getShared, dumpBytecode) to static.

Comment 44

[Tooru Fujisawa [:arai]]

Attachment: Part 24: Change NativeObject::{fillInAfterSwap,replaceWithNewEquivalentShape,generateOwnShape,shadowingShapeChange} to static method.

Removed `selfRoot` from NativeObject::replaceWithNewEquivalentShape.
also changed depending methods (fillInAfterSwap, generateOwnShape, shadowingShapeChange) to static.

Comment 45

[Tooru Fujisawa [:arai]]

Attachment: Part 25: Change NativeObject::toDictionaryMode to static method.

Removed `self` from NativeObject::toDictionaryMode

Comment 46

[Tooru Fujisawa [:arai]]

Attachment: Part 26: Remove Shape::set declaration.

Just removed unused and undefined declaration.

Comment 47

[Tooru Fujisawa [:arai]]

Attachment: remaining non-static methods that takes JSContext/ExclusiveContext

so far, there are several more non-static methods that takes JSContext/ExclusiveContext.
but none of them have `self` and looks like most of them are totally safe.
so, maybe changing all of them to static method is a bit overkill.
now wondering whether I should stop close this bug by landing Part 26, or continue.

Comment 48

[Jan de Mooij [:jandem]]

Comment on attachment 8833739 [details] [diff] [review]
Part 26: Remove Shape::set declaration.

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

Good find.

Comment 49

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3418481c2d59a50e8a8b807aecf7ded05de845
Bug 1320408 - Part 22: Remove JSContext* parameter from ProxyObject::renew and Wrapper::Renew. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/7aaa5ded3d62fb9d225fe7cf33aa2cf11abd0b4b
Bug 1320408 - Part 23: Change RegExpObject::{getShared,createShared,dumpBytecode} to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/74cd437eda30065351b433ed6f09b2db0455a685
Bug 1320408 - Part 24: Change NativeObject::{fillInAfterSwap,replaceWithNewEquivalentShape,generateOwnShape,shadowingShapeChange} to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ca00e7678638e1cb92ff202e8dfadfcb866f8d2
Bug 1320408 - Part 25: Change NativeObject::toDictionaryMode to static method. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/b28f8c1a3cd846ae58d5dc50a473b1ec11119152
Bug 1320408 - Part 26: Remove Shape::set declaration. r=jandem

Comment 50

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7c3418481c2d
https://hg.mozilla.org/mozilla-central/rev/7aaa5ded3d62
https://hg.mozilla.org/mozilla-central/rev/74cd437eda30
https://hg.mozilla.org/mozilla-central/rev/2ca00e767863
https://hg.mozilla.org/mozilla-central/rev/b28f8c1a3cd8

Comment 51

[Tooru Fujisawa [:arai]]

I think it's better closing this. and file another bug for remaining ones if necessary.