Bugzilla

Comment 3

•

16 years ago

Can't reproduce this on a current nightly (2008030204) or on the version Vlad reported with (2008021904). Tried using my normal dirty profile with both versions, and with a clean profile with the old version. Is the suspicion that this only occurs with bad network connections?

http://crash-stats.mozilla.com shows only 7 reports containing "MarkSharpObjects" in the last month, but all are from 3.0b2 users. Not sure how trustworthy that search is with a stack like this (and was Breakpad even getting triggered for you?), though.

Comment 4

•

16 years ago

please note that we don't have working js symbols for windows (bug 420759).

Comment 5

•

16 years ago

(In reply to comment #0)

> (gdb) p DumpJSStack()
> 0 sss_saveState(aUpdateAll = undefined)
> ["file:///..../nsSessionStore.js":1889]
>     oState = [object Object]
>     this = [object Object]

Line 1889 in that version of the file is:

  this._writeFile(this._sessionFile, oState.toSource());

Judging from frame 18491, it's the |oState.toSource()| causing the recursion...

> #18488 0x001f358b in MarkSharpObjects (cx=0x654ab0, obj=0x150d89e0, idap=0x0)
> at ../../../mozilla/js/src/jsobj.c:410
> #18489 0x001f358b in MarkSharpObjects (cx=0x654ab0, obj=0x1b0d8ea0,
> idap=0xbfffc994) at ../../../mozilla/js/src/jsobj.c:410
> #18490 0x001f3752 in js_EnterSharpObject (cx=0x654ab0, obj=0x1b0d8ea0,
> idap=0xbfffcaec, sp=0xbfffcae8) at ../../../mozilla/js/src/jsobj.c:467
> #18491 0x001f3d06 in obj_toSource (cx=0x654ab0, argc=0, vp=0x66fce8) at
> ../../../mozilla/js/src/jsobj.c:636

Interestingly, this call site in obj_toSource() had just been modified a few days prior to the build Vlad was using, via bug 417893 and bug 416931.

I assume MarkSharpObjects has something to do with sharp variables, but I'm not sure if/why/how the object session store is calling .toSource() on would have cyclic references, or why the recursion seems to be finding ~20,000 different (?) objects to step through. Smells like a JS bug?

Assignee: dolske → general

Component: Session Restore → JavaScript Engine

Flags: blocking-firefox3+

Product: Firefox → Core

QA Contact: session.restore → general

Target Milestone: --- → mozilla1.9

Mike Shaver (:shaver -- probably not reading bugmail closely)

Comment 6

•

16 years ago

[Resetting lost blocking-ff3+ flag]

Flags: blocking1.9?

Comment 7

•

16 years ago

The objects are indeed different in our stack analysis, and we do suspect that the loads not having completed by the time the session-store save fires.

Are you able to repro with vlad's session store file?  Wasn't clear from your comment.  Turning down the timer on the initial session-store save could make it easier, as could working out of the Toronto office or another 3rd-world network location.  Perhaps mconnor needs to send you to visit?

*Could* be a JS engine bug, but the callsite changes to obj_toSource in 417893 are v. unlikely to result in this sort of behaviour, since they only affect the initial object.  (I don't see a relevant change in 416931, could be missing it?)  I suspected Array enumeration woes when we spent an hour in the debugger with it, but the properties that were being accessed were indeed real -- in our obviously-limited sampling.  If I knew how to script gdb to print state from every calling frame, I'd have been able to find more detail, but the 20K-deep object graph was making Vlad's laptop somewhat sluggish to work with.

Comment 8

•

16 years ago

(In reply to comment #7)

> Are you able to repro with vlad's session store file?

I was using the attached sessionstore.js when trying to reproduce.

> Turning down the timer on the initial session-store save could make
> it easier

That might be interesting. I'll try again with a build set to aggressively save state.

> (I don't see a relevant change in 416931, could be missing it?)

Hmm, thought I saw back-to-back changes right there when poking around in Bonsai, but looking again I don't see it.

> I suspected Array enumeration woes when we spent an hour in the debugger
> with it, but the properties that were being accessed were indeed real

Were you able to determine anything about the name/value of those properties? EG, did sessionstore think there were a billion windows to save? A trillion tabs?

Comment 9

•

16 years ago

Hmm, still no dice on reproducing. I basically hacked up a copy of nsSessionStore.js to call saveState() from a 50ms repeating timer, and launched the browser with the bad session attached to this bug. Then repeated 10x. No hang with either a trunk debug build or the 2008021904 nightly (with my hacked nsSessionStore.js).

I notice that sessionstore.js does save information for frames in a page [and two of the pages from the bad session have such stored frames]... I suppose you could risk getting some recursion going via nested frames, but it seems unlikely that you'd be able to get so many (20,000+) as to cause the symptoms here. [And, IIRC, Gecko already has safeguards against this?]

Comment 10

•

16 years ago

I took a closer look at some of the breakpad reports...

It won't let me search back more than 3 months, but that's to capture part of the period between FF3 Beta 2 and Beta 3 (2/12). 18 reports, both Windows and OS X. Simetimes 2 crashes a day, sometimes days between crashes. Most have a stack similar to this bug... Timer fires, obj_toSource, deep MarkSharpObjects recursion.

One had a rather interesting change in pattern... 

bp-1ef0736e-d90c-11dc-85ee-001a4bd46e84

This one fired a timer, did ~1700 MarkSharpObjects frames, then abruptly decided to display a modal dialog, then fired a timer again, did ~800 frames of MarkSharpObjects, and then croaked. That's, uhh, odd.

The rest of the reports had completely different stacks, so are probably different bugs...

bp-05e59c61-cc6d-11dc-846a-001a4bd46e84 (JS_CallTracer, shallow looping)
bp-612cc828-da7d-11dc-af81-001a4bd43ef6 (same)
bp-0e264b75-d132-11dc-b3b5-001a4bd43ed6 (via array_join)
bp-31ecde0c-e306-11dc-9f86-001a4bd43ed6 (via CC MarkRoots / Traverse)

[Hmm, are breakpad report IDs supposed to have so many bits in common?]

Comment 11

•

16 years ago

the modal dialog is easy, it's the slow script dialog :). SessionStore needs to protect itself from reentry. i can think of only two cases for how it would reenter, the first is this, and the second is if it were actually being debugged.

Comment 12

•

16 years ago

Hmm, that scares me a bit... (1) I thought such warnings for chrome wasn't supposed to happen, and (2) that's rather unexpected reentrancy that could have security implications (trick some sec-sensitive code into running slowly, slow-script interrupt it into an inconsistent state).

Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)

Comment 13

•

16 years ago

it's always been this way. if the ui freezes, you really don't want it to stay dead. much better to toss up the dialog, and if the dialog does make it up, you can trigger a debugger and fix it!

as for unexpected um.... most of our code is broken, but it's a standard form of broken, if you are running js outside of dom (this means any component, or any c++ that triggers a component that might be js or a proxy object, or a native widget [e.g., filepicker]) then your code almost certainly can reenter if it has an entry point. necko is known to be broken in a way like this (relates to "asyncOpen" with a contract that js impls can not properly implement w/o a c++ component that I never got added to the tree [nsDispatchAtDeath or something])

Wayne Mery (:wsmwk)

Updated

•

16 years ago

Severity: normal → critical

Keywords: crash

Damon Sicore (:damons)

Comment 14

•

16 years ago

Adding this back to the blocker list w/ P2.  Dolske, any luck reproducing?

Vlad, still seeing this?

Flags: blocking1.9? → blocking1.9+

Reporter

Comment 15

•

16 years ago

Yes, I have it again, in yesterday's nightly.  I have it open in a debug build at the moment, but there's no JS hacker around who can debug it.  I'm going to try to gather more data.

Damon Sicore (:damons)

Comment 16

•

16 years ago

Cool.  Thanks for following up, Vlad.

Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)

Reporter

Comment 17

•

16 years ago

Ok, this was deep in js object land, and brendan wasn't around.  One theory is that this is caused by closing a window (or maybe a tab?) while session store is still loading the initial set of tabs.  However, I wasn't able to reliably reproduce that, so no further clues here.  Looking at crash reports, now that we have JS symbols, there's a whole pile of these.

Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)

Reporter

Comment 18

•

16 years ago

What's going on is that we're trying to serialize the _tab property, which is attached to the tab data object that also holds "entries" and "index".  _tab is an actual XUL tab object, complete with pointers to chrome windows and the like.  When session store triggers a save while restore is still happening and restored tabs are still loading, sss_collectTabData explicitly returns the already-existing object back, with _tab property.  We then eventually call toSource() in saveState, try to serialize a really humongous graph, and run out of stack.

Also, there are a few functions (which have no callers) that pass "(" + aState + ")" to restoreWindow -- I don't think this will have the desired effect, because if aState is an object, this will just be "([Object object])".

Back over to Firefox/Session Store.  There are a couple of possible fixes here; the safest would be in sss_saveState, to explicitly walk oState and copy properties over into a new object, keeping only property names that we know about ("entries", "url", "ID", "index", "scroll", etc.).  Less safe would be to figure out some way to just get rid of _tab -- perhaps manage _tab in a separate data structure, instead of hanging it off the tab data structure.

This might be fallout from bug 393716 or bug 407162, but those also redid chunks of this, so the problem may have been present earlier.

Assignee: general → nobody

Component: JavaScript Engine → Session Restore

Flags: blocking1.9+

OS: Mac OS X → All

Product: Core → Firefox

QA Contact: general → session.restore

Target Milestone: mozilla1.9 → ---

Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)

Reporter

Updated

•

16 years ago

Flags: blocking-firefox3+

Mike Shaver (:shaver -- probably not reading bugmail closely)

Comment 19

•

16 years ago

(In reply to comment #18)
> What's going on is that we're trying to serialize the _tab property

Serializing a XUL tab results in "{}" on the latest nightly (and has done so since Firefox 1.5). What's different in your case?

> try to serialize a really humongous graph, and run out of stack.

Mind to elaborate?

> Also, there are a few functions (which have no callers) that pass "(" + aState
> + ")" to restoreWindow

That's alright - aState is a JSON string (as we can't easily pass JS objects through XPCOM).

> the safest would be in sss_saveState, to explicitly walk oState and copy
> properties over into a new object,

That would mean using JSON internally as well (cf. bug 387859).

> This might be fallout from bug 393716 or bug 407162

Much rather of bug 367605, although in my book pure JS code shouldn't be able to crash anything. What about the bugs mentioned in comment#5?

Updated

•

16 years ago

Depends on: 423443

Mike Shaver (:shaver -- probably not reading bugmail closely)

Comment 20

•

16 years ago

We should probably check recursion in MarkSharpObjects to turn this from a crash to a JS exception -- filed bug 423443 for that.

Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)

Reporter

Comment 21

•

16 years ago

(In reply to comment #19)
> (In reply to comment #18)
> > What's going on is that we're trying to serialize the _tab property
> 
> Serializing a XUL tab results in "{}" on the latest nightly (and has done so
> since Firefox 1.5). What's different in your case?

I have no idea, I'm just going by what I see in the debugger, which is serializing the _tab property, which is a XUL element, and then walking its properties.

> > try to serialize a really humongous graph, and run out of stack.
> 
> Mind to elaborate?

I don't really have anything more here other than what I said earlier -- we are somehow descending way too deep in the serialization.

> > Also, there are a few functions (which have no callers) that pass "(" + aState
> > + ")" to restoreWindow
> 
> That's alright - aState is a JSON string (as we can't easily pass JS objects
> through XPCOM).

restoreWindow checks whether it's a string or not; also, in some cases I'm pretty sure aState is an object and not a string (an object decoded from JSON).

> > the safest would be in sss_saveState, to explicitly walk oState and copy
> > properties over into a new object,
> 
> That would mean using JSON internally as well (cf. bug 387859).

Why?  You make a list of allowed property names, and you have some function that walks the objects recursively, only copying over properties that are on the allowed list.

> > This might be fallout from bug 393716 or bug 407162
> 
> Much rather of bug 367605, although in my book pure JS code shouldn't be able
> to crash anything. What about the bugs mentioned in comment#5?

JS code isn't really crashing anything -- we are running out of stack space, thus leading to a crash.  I'm not sure about either of the bugs in comment #5 -- the first one at least I'm pretty sure doesn't back out easily at the moment.

Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)

Comment 22

•

16 years ago

(In reply to comment #21)

> I have no idea, I'm just going by what I see in the debugger, which is
> serializing the _tab property, which is a XUL element, and then walking its
> properties.

Hmm, is it possible you have an extension that's tacking some extra property onto the element, and that's what triggers the problem? That would also explain why you seem to be able to easily reproduce this.

Reporter

Comment 23

•

16 years ago

Could be -- I'll try to repro with a clean profile.  I have a handful of extensions installed -- Adblock Plus, DOMI, Firebug (disabled by default), Greasemonkey, Stylish.

Comment 24

•

16 years ago

(In reply to comment #18)

> When session store triggers a save while restore is still happening and
> restored tabs are still loading, sss_collectTabData explicitly returns the
> already-existing object back, with _tab property.

I can hit that case [the else if (...) { return browser.parentNode.__SS_data; }] quite a few times when trying to reproduce, but everything works fine. So, that alone doesn't seem to be the cause.

Interestingly, the nsISessionStore getXXXState() interfaces all use toJSONString(), which explicitly filters out _tab and _hosts properties. But when we save the state ourselves to sessionstore.js, we don't use those interfaces (perf?), and just use toSource().

I tried switching toSource to toJSONString, but it appears the format you get is slightly different and we can't restore from it (maybe it works for an external caller using setBrowserState()? Yay for not using our own interfaces...) The general difference is that properties and values are all string quoted -- "width":"123" instead of width:123 -- but I didn't look at exactly why this format doesn't restore via sessionstore.js.

I'm not familiar with the perf history here (other than it was a tedious grind at times), but switching to use our own public interfaces would otherwise seem like the right thing to do, and sounds like it might fix the issue Vlad's hitting too.

Henrik Skupin [:whimboo][⌚️UTC+1]

Comment 25

•

16 years ago

(In reply to comment #24)
> I tried switching toSource to toJSONString, but it appears the format you get
> is slightly different and we can't restore from it

See bug 387859 for how such a patch would have to look like.

And the reason we haven't done this, so far, is indeed performance: as long as our new native JSON implementation doesn't support white- or blacklisting, we'd have to make a copy of the internal data structure in JS which is significantly slower than just calling toSource() and letting it filter out the non-serializable stuff.

Comment 27

•

16 years ago

I filed the same bug yesterday because I didn't find this one. I'm able to constantly reproduce it with following STR:

1. Open given website: http://www.lexus.com/models/GSh/
2. Quickly click on "Photo Gallery" while page is loading
=> Hang and crash (perhaps doesn't occur each time)

If you need any information from one of these frames I could deliver it.

URL: http://www.lexus.com/models/GSh/

Status: NEW → ASSIGNED

Henrik Skupin [:whimboo][⌚️UTC+1]

Comment 28

•

16 years ago

The crash I was running into yesterday is: bp-ecfb38cf-f878-11dc-ae08-001a4bd46e84

Henrik Skupin [:whimboo][⌚️UTC+1]

Updated

•

16 years ago

Status: ASSIGNED → NEW

Mike Connor [:mconnor]

Comment 29

•

16 years ago

I don't think this is a common crash, at all, and while I'd like to fix the core issue, this doesn't feel like a blocker... Renom to discuss

Flags: blocking-firefox3+ → blocking-firefox3?

Mike Shaver (:shaver -- probably not reading bugmail closely)

Comment 30

•

16 years ago

Crash was fixed by 423443

Checking in mozJSComponentLoader.cpp;
/cvsroot/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,v  <--  mozJSComponentLoader.cpp
new revision: 1.159; previous revision: 1.158
done
.

Should get better diagnostics now, if nothing else.

Severity: critical → normal

Keywords: crash

Summary: crash on startup in session restore (with JS Stack) → session restore makes pathological object graph, possibly when interrupted during restore

Mike Connor [:mconnor]

Updated

•

16 years ago

Flags: blocking-firefox3? → blocking-firefox3-

Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)

Reporter

Comment 31

•

16 years ago

Indeed; RC1, did an undo close tab.. browser hung for a bit while it allocated 500MB, and then I got:

Error: too much recursion
Source File: file:///Applications/Firefox.app/Contents/MacOS/components/nsSessionStore.js
Line: 1896
> this._writeFile(this._sessionFile, oState.toSource());

I still think there's something very fishy going on, and that a wrong property is being iterated during the toSource.  I don't think session restore should be using toSource, and I think we should stop having it use toSource for 1.9.0.x.

Flags: wanted1.9.0.x?

Samuel Sidler (old account; do not CC)

Comment 32

•

16 years ago

For reference: The error from comment #31 has also been filed as bug 427193. Might be related to Firebug or to one of the other bugs blocking bug 429414.

Updated

•

16 years ago

Flags: wanted1.9.0.x?

Flags: wanted1.9.0.x+

Flags: wanted-firefox3.1?

Matěj Cepl

Comment 33

•

16 years ago

Attached file Backtrace from crashed firefox (obsolete) — Details

Looks to me like "me too" is in order. On restore of the session firefox (firefox-3.0.1-1.fc9.i386 on Fedora 9) quite often crashes.

(I had to eliminate 1000 identical MarkSharpObjects lines to make the backtrace under 512k limit).

Comment 34

•

16 years ago

Comment on attachment 338298 [details]
Backtrace from crashed firefox

matej: you don't have debug info for xulrunner, which makes your stack useless. please read about debuginfo-install and then use it.

Attachment #338298 - Attachment is obsolete: true

Matěj Cepl

Comment 35

•

16 years ago

(In reply to comment #34)
> (From update of attachment 338298 [details])
> matej: you don't have debug info for xulrunner, which makes your stack useless.
> please read about debuginfo-install and then use it.

Yeah, I knew -- I had some testing xulrunner which I forgot to downgrade. Yes, will try next time, sorry.

Maxx

Comment 36

•

15 years ago

(In reply to comment #5)
> (In reply to comment #0)
> 
> > (gdb) p DumpJSStack()
> > 0 sss_saveState(aUpdateAll = undefined)
> > ["file:///..../nsSessionStore.js":1889]
> >     oState = [object Object]
> >     this = [object Object]
> 
> Line 1889 in that version of the file is:
> 
>   this._writeFile(this._sessionFile, oState.toSource());
> 
> Judging from frame 18491, it's the |oState.toSource()| causing the recursion...

Hello,

With Firefox 3.0.7 I'm still having this kind of error, see the javascript errors that show up (thanks to Firebug)

too much recursion
[Break on this error] this._writeFile(this._sessionFile, oState.toSource()); [nsSessionStore.js (line 1909)]

[Exception... "Component is not available" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: file:///C:/Program%20Files/Mozilla%20Firefox/components/nsSessionStore.js :: sss_saveState :: line 1909" data: no]
[Break on this error] this._writeFile(this._sessionFile, oState.toSource());

Regards,
Maxx

Comment 37

•

15 years ago

Has this ever happened on Firefox 3.1 Beta 2 or later (with bug 407110 fixed)?