91434 - [FIX]Crash on 'history.go(0)' for page with JS-generated content

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[John Morrison]

Overview Description: 

Someone mentioned this crash in another bug report for the listed URL. 
I couldn't reproduce the crash in the actual page, but I did find this
crash after making a slight change to the content (and then hacked it 
down to this short example [to be attached]).

In the testcase, you make a selection in the left hand control. This
will call an onchange handler to set the options in the right hand
control. After that, it will call 'history.go(0)', which (I think) was
a legacy workaround for Nav4.x. This results in some bad
timer/callback fu. (In the destructor, it tries to do
|NS_IF_RELEASE(mCallback);|, but at this point |this| is 0xfeeefeee. But
I have no idea why nsFilePicker is in that stack. That's bizarre!)


nsTimer::~nsTimer(nsTimer * const 0xfeeefeee) line 122 + 10 bytes
nsTimer::`scalar deleting destructor'(nsTimer * const 0xfeeefeee, unsigned int 
1) + 8 bytes
nsFilePicker::Release(nsFilePicker * const 0x02c96008) line 45 + 28 bytes
nsTimerManager::FireNextReadyTimer(nsTimerManager * const 0x00cebad0, unsigned 
int 0) line 117 + 6 bytes
nsAppShell::Run(nsAppShell * const 0x00cebad0) line 118
nsAppShellService::Run(nsAppShellService * const 0x00c6c5a8) line 423
main1(int 1, char * * 0x00322e48, nsISupports * 0x00324280) line 1174 + 9 bytes
main(int 1, char * * 0x00322e48) line 1478 + 25 bytes
WinMain(HINSTANCE__ * 0x00400000, HINSTANCE__ * 0x00400000, char * 0x0013322e, 
HINSTANCE__ * 0x00400000) line 1496 + 21 bytes
MOZILLA! WinMainCRTStartup + 308 bytes
KERNEL32! 77e87903()


although sometimes, the stack is in image layout code. 

nsImageBoxFrame::UpdateImage(nsImageBoxFrame * const 0x00000005, nsIPresContext 
* 0x01dd9bd0, int & 0) line 263 + 21 bytes
nsImageBoxFrame::DidSetStyleContext(nsImageBoxFrame * const 0x00000000, 
nsIPresContext * 0x01dd9bd0) line 403
nsFrame::SetStyleConte01d6c790, nsIConpedJSClass * consthannel * 0x01ffcc40, s
XPC_WN_CallMethodus * 0x0012f498) line 1762 + 16 bytes
nsGenericElement::HandleDOMEvent(nsGenericElement * const 0x03660110, 
nsIPresContext * 0x01ffaa60, nsEvent * 0x03659f98, nsIDOMEvent * * 0x0012f38c, 
unsigned int 1, nsEventStatus * 0x0012f498) line 1674
... etc.



Steps to Reproduce: 
1) Make a selection in the left hand control.
2) Pray.
3) If still alive, try again (about 80% kill rate).

Actual Results: crashes with the stack above (or the other one :)

Expected Results: changes the selection, does reload.

Build Date & Platform Bug Found: win2k trunk build 7/17

Additional Builds and Platforms Tested On: none yet

Additional Information: 

If I wrap the 'history.go' in a setTimeout, I don't crash. However, 
the new js-generated content is blown away from the second control.
(I haven't really considered whether that's a bug or not; not sure
what the rules are for that case).

Comment 1

[John Morrison]

Attachment: testcase; make a selection in LH control and crash (usually)

Comment 2

[John Morrison]

Attachment: USE THIS ONE; make a selection in LH control and crash (usually)

Comment 3

[John Morrison]

Apparently this only crashes on Windows.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I've only seen the ~nsTimer stack so far (twice).  When I've seen it, the |this|
in the destructor looked OK, but |mCallback| pointed to something whose vtable
pointer was 0xdddddddd, or deleted.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm guess it's related to the form controls, but I could certainly be wrong.

Comment 6

[rods (gone)]

Attachment: patch

Comment 7

[rods (gone)]

There were two issues here:

1) The mUpdateTimer was getting initialized once for each item and this was 
causing it to get multiply ref counted.

2) mUpdateTimer was getting deleted instead of released

Comment 8

[rods (gone)]

Attachment: Lastest patch for this and Bug 92458

Comment 9

[rods (gone)]

This is a better patch than the one above, once the timer is cancelled you can't
restart is without an Init which also addrefs. 

So now we do the Init each time, but we do a release before the Init so the
counts are correct. (the very last part of the patch is for Bug 92458)

Comment 10

[rods (gone)]

Attachment: New testcase for this and Bug 92458 (ignore link at bottom of page)

Comment 11

[dcone (gone)]

r=dcone provided you add the changes for 92458 and the comments to the last 
section.

Comment 12

[Marc Attinasi]

I'm wondering about that RELEASE just before the call to 

  mTimer->Init(this, mDelay);

Shouldn't the counter-balancing RELEASE be done AFTER the call to Init? I'm
worried that the RELEASE before the call to Init could cause it to be destroyed,
whereas calling it after will just remove the extra refcount that came from the
Init call.

Comment 13

[Marc Attinasi]

Rod explained (phone) that the refcount is always going to be greater than 1 at
the point where the RELEASE is called because it is addref'd  first by the
creator, and second by the first call to Init. So, he agreed to add an assertion
that the refcount is > 1 before the RELEASE, and I am happy with that
(documentation is good). sr=attinasi

Comment 14

[rods (gone)]

fixed

Comment 15

[Madhur Bhatia]

verified