346484 - Crash (often closing firefox) [@ RunCloseHooks]

Status: RESOLVED DUPLICATE of bug 341815

(Core :: JavaScript Engine, defect)

Description

[dolphinling]

I've crashed 3 times with the 2006-07-28-04 linux trunk build, and talkback shows my crash is the #4 topcrash in only one day. 

Some talkbacks: TB21577734 TB21577780 TB21577783

(from the talkbacks list, it's still happening in 07-29 builds, too)

Comment 1

[dolphinling]

I'm just gonna take a *guess* that this is a regression from bug 341821, which had a checkin comment of "Close hooks are run outside GC locks". CCing relevant people from that bug.

Comment 2

[Brian Polidoro]

Is this the same as bug 341815?

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> Is this the same as bug 341815?
> 

I do not know: the crash there is causes by a specially crafted test case, which is not case here.

Comment 4

[Ria Klaassen (not reading all bugmail)]

Steps to reproduce:

* Install Adblock Plus: https://addons.mozilla.org/firefox/1865/
* Set two prefs (sometime it fails without the second one):
user_pref("browser.startup.homepage", "http://www.nu.nl/");
user_pref("browser.sessionstore.resume_session_once", true);
* Open a second tab with a link or bookmark
* It will crash on close: TB21600142E

Regression between 1.9a1_2006072823 and 1.9a1_2006072904:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-07-28+22%3A00&maxdate=2006-07-29+05%3A00

Comment 5

[Brendan Eich [:brendan]]

Igor, I thought I was following your changes when I reviewed, so could be forgetting something, but now that I look at jsscript.c revs 3.105-3.107 (current), I do not see why marking script filenames should be conditioned on keepAtoms in any case.  What is the rationale for that change?

/be

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> Igor, I thought I was following your changes when I reviewed, so could be
> forgetting something, but now that I look at jsscript.c revs 3.105-3.107
> (current), I do not see why marking script filenames should be conditioned on
> keepAtoms in any case.

Previously the condition was "(gcflags & GC_KEEP_ATOMS)" and the currently applied patch has not change it. I presume the condition is necessary since the last ditch GC can otherwise collect not yet rooted scripts which would not be mark with explicit js_MarkScriptFilename call?



But there is no such conditions in the current CVS, that was temporary 

>  What is the rationale for that change?
> 
> /be
> 

Comment 7

[Igor Bukanov]

(In reply to comment #4)
> * It will crash on close: TB21600142E

I figured why the patch from bug causes the crash. There is always at least one close hook installed, which is do-nothing hook for hidden Genarator.prototype. During GC loop on shutdown the corresponding object becomes unrooted at some point and would be transfered to  JSContext.gcObjectsToClose for execution. Later the finalization starts and one of finilzers would call RemoveRoot. That would set gcPoke and trigger GC restart. 

Now since the patch from 341821 moved finalizers outside gcloop, on the next gc cycle it is necessary to mark JSContext.gcObjectsToClose which the patch did. The trouble is that it was done during enumeration loop for all context. But that would not enumerate the current context as it would be already unlinked from the global list. Hence JSContext.gcObjectsToClose would be GC-ed leading exactly for the crash with reported stack traces.

This is yet another reason to prevent running close hooks on GC invoked from js_DestroyContext.

Comment 8

[Brendan Eich [:brendan]]

Can this bug and bug 341815 be combined by duping one against the other?

/be

Comment 9

[Igor Bukanov]

*** This bug has been marked as a duplicate of 341815 ***