Igor Bukanov Assignee
	Description • 13 years ago

After the bug 674251 which turns JSScript into JSObject we no longer need a special object that holds a script to ensure GC rooting. We should eliminate those objects.

	Igor Bukanov Assignee
	Comment 1 • 13 years ago

It is not that straightforward to remove the script objects. Currently we use them to find the global for the script for compartment and principals management. So I will leave true elimination of script objects to another bug. For this bug my plan is to continue to create the script objects internally but always use JSScript, not its JSObject* holder, in CompileScript/EvaluateScript API.

	Igor Bukanov Assignee
	Comment 2 • 13 years ago

This is what I have sent to the try server

	Igor Bukanov Assignee
	Comment 3 • 13 years ago

v1 has a leak that is a regression from the bug 674251. In that bug I forgot to update the cycle collector to treat JSScript as a potential source of cycles. The changes in this patch that pass JSScript to AddRoot/LockGCThing exposed that.

In most places the patch just replaces JSObject *scriptObj with JSScript *script. But in js shell, where the disassembling implementation currently converts the script object into jsval to reuse some code, a refactoring was necessary.

	Jason Orendorff [:jorendorff]
	Comment 4 • 13 years ago

Comment on attachment 558090 [details] [diff] [review]
v2

Pretty straightforward. This is great to see. Sorry for the slow review--I'm in San Jose and it has been a busy week here.

>+    if (script->hasFunction) {
>+        JSFunction *fun = script->function();

Does this work on a script that has never been executed? I thought that stuff was lazily populated by the type inference engine. JSScript::function() asserts not just hasFunction but (hasFunction && types).

>+struct DisassembleOptionParser {

Thank you. Very nice. :)

	Igor Bukanov Assignee
	Comment 5 • 13 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=678830 - pushed with a fix not to use script->function() in the disassembling shell functions. Instead the patch uses function pointers that are supplied by the caller.

	(no longer active)
	Comment 6 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/5a3e49205389

	Matt Brubeck (:mbrubeck)
	Comment 7 • 13 years ago

This patch caused a new intermittent memory leak in mochitest-chrome leaks.  See bug 687857 comment 123 for details.  This leak is the currently #1 cause of test failures, with over 213 failures in the past 28 days on mozilla-inbound alone:
http://brasstacks.mozilla.com/orangefactor/?display=&tree=mozilla-inbound

Can we back out this patch?  (Are there other patches that have landed since that depend on it?)  Who can investigate the leak?

	Igor Bukanov Assignee
	Comment 8 • 13 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> Can we back out this patch?

So far no patches that depends on this has landed, so we should be able to back this out.

	Matt Brubeck (:mbrubeck)
	Comment 9 • 13 years ago

(In reply to Igor Bukanov from comment #8)
> So far no patches that depends on this has landed, so we should be able to
> back this out.

Would you like me to back this out, Igor, or would you prefer to do it yourself?

	Igor Bukanov Assignee
	Comment 10 • 13 years ago

I can do the back out, but can we wait with it until Tuesday?

	Matt Brubeck (:mbrubeck)
	Comment 11 • 13 years ago

(In reply to Igor Bukanov from comment #10)
> I can do the back out, but can we wait with it until Tuesday?

Sure.

This patch is on Aurora too; should we request approval to back it out there? (I have no idea whether the test failures indicate a real bug in this patch or if it's just exposing an existing problem elsewhere.)

	Igor Bukanov Assignee
	Comment 12 • 13 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #11)
>(I have no idea whether the test failures indicate a real bug in this
> patch or if it's just exposing an existing problem elsewhere.)

I suspect that with the patch there are more chances that the native stack scanner for the conservative GC finds a JSScript instance that otherwise should be dead leading to the leak.

	Igor Bukanov Assignee
	Comment 13 • 13 years ago

It is not simple to back out the patch as after its landing we have landed the refactoring patches that caused a lot of conflicts. I will try too investigate this more. If nothing would come out until FRiday, then I will create a back out patch and ask for its review.

	Matt Brubeck (:mbrubeck)
	Comment 14 • 13 years ago

In case it helps with leak hunting, we also saw a definite increase in bug 653080 (a leak in mochitest-browser-chrome) when this landed:
http://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=653080&startday=2011-09-07&endday=2011-10-05&tree=mozilla-inbound

	Igor Bukanov Assignee
	Comment 15 • 13 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #14)
> In case it helps with leak hunting, we also saw a definite increase in bug
> 653080 (a leak in mochitest-browser-chrome) when this landed:
> http://brasstacks.mozilla.com/orangefactor/
> ?display=Bug&bugid=653080&startday=2011-09-07&endday=2011-10-05&tree=mozilla-
> inbound

Hm, the current spike in oranges is significantly smaller than the one that was observed in August and that ended at the beginning of September. Do we have any explanation for that? For me it looks that currently we ended up currently in the state that existed before the beginning of August.

	Matt Brubeck (:mbrubeck)
	Comment 16 • 13 years ago

(In reply to Igor Bukanov from comment #15)
> Hm, the current spike in oranges is significantly smaller than the one that
> was observed in August and that ended at the beginning of September. Do we
> have any explanation for that? For me it looks that currently we ended up
> currently in the state that existed before the beginning of August.

No, I haven't seen any investigation or explanation of the August spike in bug 653080.

Note that the current spike in bug 687857 is just as severe as the August spike in 653080:
http://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=687857&startday=2011-07-01&endday=2011-10-05&tree=mozilla-inbound

	Igor Bukanov Assignee
	Comment 17 • 13 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #16)
> (In reply to Igor Bukanov from comment #15)
> > Hm, the current spike in oranges is significantly smaller than the one that
> > was observed in August and that ended at the beginning of September. Do we
> > have any explanation for that? For me it looks that currently we ended up
> > currently in the state that existed before the beginning of August.
> 
> No, I haven't seen any investigation or explanation of the August spike in
> bug 653080.

So at worst this bug returned the situation to one that we had in August and we have no information regarding what started that and what stopped that, right? Can we get some correlations for that spike?

	Igor Bukanov Assignee
	Comment 18 • 13 years ago

I do not think it is productive to back out this bug. Rather I would prefer to spend time developing tools for better leak reporting for JS GC heap, see bug 692914 and then go back to investigate the intermittent leaks. I will work on that bug when I come back from a vacation,

	Matt Brubeck (:mbrubeck)
	Comment 19 • 13 years ago

(In reply to Igor Bukanov (offline 2011-10-08 -- 2011-10-15) from comment #18)
> I do not think it is productive to back out this bug.

It seems unusual to me that we would leave a patch in the tree that does not pass our automated tests - especially when it is the top cause of intermittent failures.  If someone had thought to retrigger tests sooner, we would have just backed this out without asking.  (If nothing else, this makes much more work for people watching the tree, because it occurs about a hundred times a week and TBPL does not automatically mark memory leak bugs.)

(In reply to Igor Bukanov (offline 2011-10-08 -- 2011-10-15) from comment #18)
> Rather I would prefer to spend time developing tools for better leak reporting
> for JS GC heap, see bug 692914 and then go back to investigate the intermittent
> leaks.

It's not clear to me from reading bug 692914: Will that resolve the intermittent leaks?  Or will it make them easier to resolve?  Or will it fix poor reporting that made the intermittent leaks go undetected before this patch landed?

Basically, I want to know the plan and estimated schedule for resolving bug 687857, and the rationale for leaving it in the tree in the meantime.  If someone else who understands this code can answer while Igor is on vacation, I'd appreciate it.

	Matt Brubeck (:mbrubeck)
	Comment 20 • 13 years ago

Bug 687857 seems to have stopped completely as of 8 October.  I have no idea why - maybe because Igor went on vacation!  :)  If I have time to run hundreds of Try jobs again soon, maybe I will try bisecting to find exactly when it disappeared.

Anyway, if the leaks don't come back then we can consider them fixed and we no longer need to back out this patch.

	Igor Bukanov Assignee
	Comment 21 • 13 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #19)
> (In reply to Igor Bukanov (offline 2011-10-08 -- 2011-10-15) from comment
> #18)
> > I do not think it is productive to back out this bug.
> 
> It seems unusual to me that we would leave a patch in the tree that does not
> pass our automated tests - especially when it is the top cause of
> intermittent failures.  If someone had thought to retrigger tests sooner, we
> would have just backed this out without asking.

I should have write more clear: the amount of work to backout the patch appeared to be substantial as other patches touched the same code. Given that the spike correlated with this bug is not worse then the one from August that appeared and disappeared for unknown reasons my assumption was that some efforts with leak reporting would give a chance at least to understand the reason for the spike and potentially fix the bug.


> It's not clear to me from reading bug 692914: Will that resolve the
> intermittent leaks?  Or will it make them easier to resolve?  Or will it fix
> poor reporting that made the intermittent leaks go undetected before this
> patch landed?

Currently we do not report leaks in the JS runtime at all. The garbage collector just assume on shutdown that everything is the garbage and forcefully release the memory. So we have no idea if the intermittent failures are related to anything in the JS world. And this is what I want to know. But I do not know if the answer would help with fixing the leak.