Last Comment Bug 678830 - Use JSScript, not script objects, in compile/evaluate API.
: Use JSScript, not script objects, in compile/evaluate API.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal with 1 vote (vote)
: mozilla9
Assigned To: Igor Bukanov
:
Mentors:
Depends on: 653080 674251 687857 699796
Blocks: 662704 684529 692914
  Show dependency treegraph
 
Reported: 2011-08-14 07:39 PDT by Igor Bukanov
Modified: 2011-11-04 07:38 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (92.63 KB, patch)
2011-09-02 07:08 PDT, Igor Bukanov
no flags Details | Diff | Review
v2 (94.11 KB, patch)
2011-09-03 12:30 PDT, Igor Bukanov
jorendorff: review+
Details | Diff | Review

Description Igor Bukanov 2011-08-14 07:39:44 PDT
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.
Comment 1 Igor Bukanov 2011-09-02 03:48:42 PDT
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.
Comment 2 Igor Bukanov 2011-09-02 07:08:04 PDT
Created attachment 557822 [details] [diff] [review]
v1

This is what I have sent to the try server
Comment 3 Igor Bukanov 2011-09-03 12:30:54 PDT
Created attachment 558090 [details] [diff] [review]
v2

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.
Comment 4 Jason Orendorff [:jorendorff] 2011-09-16 10:52:56 PDT
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. :)
Comment 5 Igor Bukanov 2011-09-20 01:56:58 PDT
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.
Comment 6 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-20 07:50:52 PDT
https://hg.mozilla.org/mozilla-central/rev/5a3e49205389
Comment 7 Matt Brubeck (:mbrubeck) 2011-10-02 09:07:25 PDT
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?
Comment 8 Igor Bukanov 2011-10-02 13:02:48 PDT
(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.
Comment 9 Matt Brubeck (:mbrubeck) 2011-10-02 15:29:43 PDT
(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?
Comment 10 Igor Bukanov 2011-10-03 01:45:50 PDT
I can do the back out, but can we wait with it until Tuesday?
Comment 11 Matt Brubeck (:mbrubeck) 2011-10-03 07:29:32 PDT
(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.)
Comment 12 Igor Bukanov 2011-10-04 02:41:28 PDT
(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.
Comment 13 Igor Bukanov 2011-10-05 03:46:00 PDT
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.
Comment 14 Matt Brubeck (:mbrubeck) 2011-10-05 10:00:07 PDT
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
Comment 15 Igor Bukanov 2011-10-05 12:56:44 PDT
(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.
Comment 16 Matt Brubeck (:mbrubeck) 2011-10-05 13:15:03 PDT
(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
Comment 17 Igor Bukanov 2011-10-05 13:48:49 PDT
(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?
Comment 18 Igor Bukanov 2011-10-07 13:41:46 PDT
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,
Comment 19 Matt Brubeck (:mbrubeck) 2011-10-09 18:52:22 PDT
(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.
Comment 20 Matt Brubeck (:mbrubeck) 2011-10-11 14:38:28 PDT
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.
Comment 21 Igor Bukanov 2011-10-24 03:20:29 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.