Closed Bug 1320408 Opened 3 years ago Closed 2 years ago

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

Categories

(Core :: JavaScript Engine, defect, P3)

54 Branch
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(27 files)

6.82 KB, patch
jandem
: review+
Details | Diff | Splinter Review
37.99 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.67 KB, patch
jandem
: review+
Details | Diff | Splinter Review
24.97 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.69 KB, patch
jandem
: review+
Details | Diff | Splinter Review
7.10 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.11 KB, patch
jandem
: review+
Details | Diff | Splinter Review
5.33 KB, patch
jandem
: review+
Details | Diff | Splinter Review
26.21 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.04 KB, patch
jandem
: review+
Details | Diff | Splinter Review
6.59 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.61 KB, patch
jandem
: review+
Details | Diff | Splinter Review
4.57 KB, patch
jandem
: review+
Details | Diff | Splinter Review
98.06 KB, patch
jandem
: review+
Details | Diff | Splinter Review
9.94 KB, patch
jandem
: review+
Details | Diff | Splinter Review
10.56 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.42 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.60 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.82 KB, patch
jandem
: review+
Details | Diff | Splinter Review
10.83 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.96 KB, patch
jandem
: review+
Details | Diff | Splinter Review
5.25 KB, patch
jandem
: review+
Details | Diff | Splinter Review
10.85 KB, patch
jandem
: review+
Details | Diff | Splinter Review
14.81 KB, patch
jandem
: review+
Details | Diff | Splinter Review
6.83 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.09 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.68 KB, text/plain
Details
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.
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).
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
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Blocks: 883377
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
Attachment #8814646 - Flags: review?(jdemooij)
Same for JSFunction::getOrCreateScript.
Attachment #8814647 - Flags: review?(jdemooij)
while checking, noticed that JSFunction::getBoundFunctionArgument doesn't need JSContext*.
so just removed the parameter.
Attachment #8814648 - Flags: review?(jdemooij)
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>();
Attachment #8814646 - Flags: review?(jdemooij) → review+
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>());
Attachment #8814647 - Flags: review?(jdemooij) → review+
Comment on attachment 8814648 [details] [diff] [review]
Part 3: Remove JSContext* parameter from JSFunction::getBoundFunctionArgument.

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

Nice find.
Attachment #8814648 - Flags: review?(jdemooij) → review+
Thank you for reviewing :)
I'll land patches incrementally.
Keywords: leave-open
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
No longer blocks: 883377
See Also: → 883377
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?
Attachment #8821449 - Flags: review?(jdemooij)
Also removed temporary fix in Part 4.
Attachment #8821450 - Flags: review?(jdemooij)
Just removed unused JSContext* parameter from shouldSplicePrototype.
Attachment #8821452 - Flags: review?(jdemooij)
(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 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?
Attachment #8821449 - Flags: review?(jdemooij) → review+
Attachment #8821450 - Flags: review?(jdemooij) → review+
Comment on attachment 8821451 [details] [diff] [review]
Part 6: Change JSObject::splicePrototype to static method.

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

Nice!
Attachment #8821451 - Flags: review?(jdemooij) → review+
Attachment #8821452 - Flags: review?(jdemooij) → review+
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
just like JSObject::reportReadOnly, changed other report methods static.
Attachment #8824668 - Flags: review?(jdemooij)
Changed JSObject::setFlags to static and removed self variable.
and also changed instance methods setSomething that calls JSObject::setFlags to static.
Attachment #8824669 - Flags: review?(jdemooij)
Changed LazyScript::functionDelazifying to static and removed self variable
Attachment #8824670 - Flags: review?(jdemooij)
Just removed unused JSContext parameter from JSScript::ensureNonLazyCanonicalFunction.
Attachment #8824671 - Flags: review?(jdemooij)
This may be unnecessary change tho, changed JSScript::sourceData to static.
Feel free to reject ;)
Attachment #8824673 - Flags: review?(jdemooij)
Changed DebugEnvironmentProxy::getMaybeSentinelValue to static and removed self variable
Attachment #8824674 - Flags: review?(jdemooij)
Changed GlobalObject::createBlankPrototype, GlobalObject::createBlankPrototypeInheriting, GlobalObject::getRegExpStatics, GlobalObject::getOrCreate*,
and removed self variables there.
Attachment #8824675 - Flags: review?(jdemooij)
Attachment #8824668 - Flags: review?(jdemooij) → review+
Attachment #8824669 - Flags: review?(jdemooij) → review+
Attachment #8824670 - Flags: review?(jdemooij) → review+
Attachment #8824671 - Flags: review?(jdemooij) → review+
Attachment #8824673 - Flags: review?(jdemooij) → review+
Attachment #8824674 - Flags: review?(jdemooij) → review+
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
Attachment #8824675 - Flags: review?(jdemooij) → review+
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
Removed `self` in NativeObject::addDataProperty.
Attachment #8831961 - Flags: review?(jdemooij)
Removed `self` in NativeObject::removeProperty.
Attachment #8831962 - Flags: review?(jdemooij)
Removed `self` in NativeObject::clearFlag.
Attachment #8831963 - Flags: review?(jdemooij)
Removed `self` in StringObject::init.
Attachment #8831964 - Flags: review?(jdemooij)
Removed unused `JSContext* cx` parameter from ModuleObject::fixEnvironmentsAfterCompartmentMerge.
Attachment #8831966 - Flags: review?(jdemooij)
PromiseObject::resolve and PromiseObject::reject don't have `self`, but still referring `this`.
So changed them to static method to prevent rooting-related issue.
Attachment #8831967 - Flags: review?(jdemooij)
Removed `promise` variable that is actually `self`, from PromiseObject::onSettled.
Attachment #8831968 - Flags: review?(jdemooij)
Attachment #8831961 - Flags: review?(jdemooij) → review+
Attachment #8831962 - Flags: review?(jdemooij) → review+
Attachment #8831963 - Flags: review?(jdemooij) → review+
Attachment #8831964 - Flags: review?(jdemooij) → review+
Attachment #8831966 - Flags: review?(jdemooij) → review+
Attachment #8831967 - Flags: review?(jdemooij) → review+
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!
Attachment #8831968 - Flags: review?(jdemooij) → review+
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
Just removed unused parameter.
Attachment #8833735 - Flags: review?(jdemooij)
Removed `self` from RegExpObject::createShared, and changed depending methos (getShared, dumpBytecode) to static.
Attachment #8833736 - Flags: review?(jdemooij)
Removed `selfRoot` from NativeObject::replaceWithNewEquivalentShape.
also changed depending methods (fillInAfterSwap, generateOwnShape, shadowingShapeChange) to static.
Attachment #8833737 - Flags: review?(jdemooij)
Removed `self` from NativeObject::toDictionaryMode
Attachment #8833738 - Flags: review?(jdemooij)
Just removed unused and undefined declaration.
Attachment #8833739 - Flags: review?(jdemooij)
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.
Attachment #8833735 - Flags: review?(jdemooij) → review+
Attachment #8833736 - Flags: review?(jdemooij) → review+
Attachment #8833737 - Flags: review?(jdemooij) → review+
Attachment #8833738 - Flags: review?(jdemooij) → review+
Comment on attachment 8833739 [details] [diff] [review]
Part 26: Remove Shape::set declaration.

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

Good find.
Attachment #8833739 - Flags: review?(jdemooij) → review+
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
Keywords: triage-deferred
Priority: -- → P3
I think it's better closing this. and file another bug for remaining ones if necessary.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Version: Trunk → 54 Branch
You need to log in before you can comment on or make changes to this bug.