1603330 - WeakRef.deref() makes the target being kept in tbpl mode even ClearKeptObjects() is called

Status: RESOLVED FIXED

(Core :: JavaScript: GC, task, P1)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

run the following test with --tbpl
js/src/jit-test/jit_test.py obj-x86_64-pc-linux-gnu/dist/bin/js --tbpl --args="--enable-weak-refs" gc/bug-1603330.js

let wr;
{
  let obj = {};
  wr = new WeakRef(obj);
  obj = null;
}
// obj is out of block scope now, should be GCed.

wr.deref(); // this adds the target of wr (obj) back
Math.sin(0); // Push another unused value on stack.

clearKeptObjects(); // a util function added in bug 1587093
gc();

assertEq(wr.deref() , undefined);  // will fail in when running jit_test in plain or root-analysis zeal mode with --tbpl flag, but pass in cgc zeal mode.

and I found out it's the line in WeakRefObject::deref keeps the target alive
https://searchfox.org/mozilla-central/rev/c52d5f8025b5c9b2b4487159419ac9012762c40c/js/src/builtin/WeakRefObject.cpp

args.rval().setObject(*wrappedTarget);

However this behaves different under zeal mode,
after clearKeptObjects() is called,
in cgc mode wr.deref() returns undefined,
in plain or root analysis mode wr.deref() still returns the obj,

I file this bug to track this.

Comment 1

[Steve Fink [:sfink] [:s:]]

I think there are two things to fix here.

First, I don't think wr.deref() should be returning undefined in cgc or any other mode, because new WeakRef should have added the target to KeptObjects (or whatever it's called.)

Second, I'm not sure this is a bug, though I think this should still be fixed. To explain: the spec assumes that the engine has invisible machinery that sometimes keeps things alive. In this case, it looks like the invisible machinery is the JS value stack. Does calling something else that sets a return value, like Math.sin(0), "fix" the problem? If so, then I don't think this technically is a bug, it's just a side effect of how things work.

But I still think it should be fixed. If Math.sin(0) works, then it seems like clearKeptObjects() should work in the same way. I don't have a shell built with clearKeptObjects() to test with, but does this

Math.cos(0);
print(clearKeptObjects());

print out 1 (or anything other than undefined)? I believe clearKeptObjects should be doing args.rval().setUndefined(), to overwrite the stack slot that is artificially keeping the target alive.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #1)

it looks like the invisible machinery is the JS value stack

It would be great to confirm whether this is the problem. But if it is, why are we marking unused stack slots? Maybe there is a good reason, I just don't know what it is.

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Jon Coppeard (:jonco) from comment #2)

(In reply to Steve Fink [:sfink] [:s:] from comment #1)

it looks like the invisible machinery is the JS value stack

It would be great to confirm whether this is the problem. But if it is, why are we marking unused stack slots? Maybe there is a good reason, I just don't know what it is.

That's a fair point, but I think it makes sense what it's doing assuming gc() is the next thing called.

Immediately after clearKeptObjects(), I'd guess this looks exactly like clearKeptObjects() returned the target. If there's a gc() call after that, then it may be that the state is indistinguishable from gc() returning the target object, up until the point where the code sets gc()'s rval. And if you set an rval then GC within a native function before returning, I would certainly expect that rval to get marked. So it kind of makes sense that the GC is considering that part of the stack to be live, and also suggests that we might want to consider clearing out the rval in GC() before doing the collection to prevent the leftover Value in the rval from being marked unnecessarily.

I think I'll simplify my opinion: there are 2 bugs here, and one enhancement for another bug.

• Bug one: new WeakMap(target) should be adding target to KeptObjects
• Bug two: clearKeptObjects() should be doing args.rval().setUndefined()
• Enhancement: clear out the rval earlier in GC(): filed bug 1603851.

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #3)
Thanks for explaining, I see what's going on now.

• Bug one: new WeakMap(target) should be adding target to KeptObjects

It does that already.

• Bug two: clearKeptObjects() should be doing args.rval().setUndefined()

Oh, I didn't see that was missing! Yes, this is a bug.

• Enhancement: clear out the rval earlier in GC(): filed bug 1603851.

Agreed.

Comment 5

[Steve Fink [:sfink] [:s:]]

(In reply to Jon Coppeard (:jonco) from comment #4)

(In reply to Steve Fink [:sfink] [:s:] from comment #3)
Thanks for explaining, I see what's going on now.

• Bug one: new WeakMap(target) should be adding target to KeptObjects

It does that already.

Ah. I was confused by the original test code, where there's an initial wr.deref():

wr.deref(); // this adds the target of wr (obj) back

that is unnecessary. I misinterpreted the description of the difference in behavior between SM(cgc) and SM(p) as referring to this wr.deref() changing behavior, but actually it meant the later wr.deref() in the assert. So yeah, ignore this one.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

I've updated the bug summary and tests according to previous comments, and to describe this bug in more detail.

1. This bug is specific to JIT mode, so if I don't run the tests with --tbpl flag, the test will pass.
   And when the tbpl flag is set, the test failed in following configs

FAILURES:
--enable-weak-refs --baseline-eager gc/bug-1603330.js
--enable-weak-refs --ion-eager --ion-offthread-compile=off --more-compartments gc/bug-1603330.js
--enable-weak-refs --blinterp-eager gc/bug-1603330.js
--enable-weak-refs --ion-eager --ion-offthread-compile=off --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads gc/bug-1603330.j

2. I also add a Math.sin(0) after the 1st deref() call, and the problem is still there.

And now I have WeakRef tests in mochitest, Bug 1609636, which doesn't have this problem.
I guess JIT keeps the return value because the 1st deref and 2nd deref are in the same function?

So far I've found that, after the call to deref is returned, the rval() will be passed to TypeMonitorResult in https://searchfox.org/mozilla-central/rev/c52d5f8025b5c9b2b4487159419ac9012762c40c/js/src/jit/BaselineIC.cpp#2913
And will be kept in ICStubs.

Perhaps I shoul find out when JIT will throw away the Stubs and rewrite the test.

Comment 7

[Steve Fink [:sfink] [:s:]]

It makes sense to me that the JIT would record the returned value, but I don't know how to prevent it. That's definitely a question for a JIT person. I'd ask jandem.

jandem: The problem appears to be that the JIT is hanging onto the return value of WeakRef#deref(), which prevents it from being collected. Is there a way to prevent that object from being retained? (It would be ok if its ObjectGroup were recorded, if that helps.)

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Steve Fink [:sfink] [:s:] from comment #7)

jandem: The problem appears to be that the JIT is hanging onto the return value of WeakRef#deref(), which prevents it from being collected. Is there a way to prevent that object from being retained? (It would be ok if its ObjectGroup were recorded, if that helps.)

Does it help to move the object-allocation code into a separate function and call that, before doing the rest?

Worst-case we could add |jit-test| --no-blinterp to the test, but it would be best to avoid that.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

Attachment: Bug 1603330 - Update test to prevent JIT keeps the retval of wr.deref().

Launch an interrupt function and call gc() to remove the reference kept
by JIT code.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16255d8df9bd
Update test to prevent JIT keeps the retval of wr.deref(). r=jonco

Comment 11

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/16255d8df9bd