495728 - ###!!! ASSERTION: destroy called on frame while scripts not blocked: '!nsContentUtils::IsSafeToRunScript()', file /Users/bzbarsky/mozilla/vanilla/mozilla/layout/generic/nsFrame.cpp, line 446

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Load the testcase in bug 495648 (after adding the relevant enablePrivilege
    call to it, etc).  Any other scrollable listbox would probably do too.
2)  Scroll the listbox down.

ACTUAL RESULTS: Lots of asserts

EXPECTED RESULTS: No asserts

ADDITIONAL INFORMATION:
#12 0x11a8a8a0 in nsBoxFrame::Destroy (this=0x15084ac) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/xul/base/src/nsBoxFrame.cpp:964
#13 0x11abdf0c in nsListBoxBodyFrame::RemoveChildFrame (this=0x14fb528, aState=@0xbfffcbb4, aFrame=0x15084ac) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/xul/base/src/nsListBoxBodyFrame.cpp:1500
#14 0x11abe23f in nsListBoxBodyFrame::DestroyRows (this=0x14fb528, aRowsToLose=@0xbfffcc10) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/xul/base/src/nsListBoxBodyFrame.cpp:1077
#15 0x11abe3fe in nsListBoxBodyFrame::DoInternalPositionChanged (this=0x14fb528, aUp=0, aDelta=7) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/xul/base/src/nsListBoxBodyFrame.cpp:915
#16 0x11ac0255 in nsListBoxBodyFrame::DoInternalPositionChangedSync (this=0x14fb528, aUp=0, aDelta=7) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/xul/base/src/nsListBoxBodyFrame.cpp:880
#17 0x11ac02fc in nsListBoxBodyFrame::InternalPositionChangedCallback (this=0x14fb528) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/xul/base/src/nsListBoxBodyFrame.cpp:845
#18 0x11ac0379 in nsListScrollSmoother::Notify (this=0x1d6d7e80, timer=0x1d6d61a0) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/xul/base/src/nsListBoxBodyFrame.cpp:141
#19 0x00570eff in nsTimerImpl::Fire (this=0x1d6d61a0) at /Users/bzbarsky/mozilla/vanilla/mozilla/xpcom/threads/nsTimerImpl.cpp:430

Looks like a regression from bug 491848, at least in terms of it having added this assert.  It's possible that we just need more scriptblocker sprinklage here.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

I think bz was right, we just need a script blocker here.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 380794 [details] [diff] [review]
patch

Why is this the right place to put the scriptblocker?  At least some of the calls to this function should already have scriptblockers on stack, and it seems like this would just hide asserts if they manage to screw it up.

Also, keep in mind that any time a script blockers goes out of scope script can run.  So this patch means that script can run for every RemoveChildFrame call in DestroyRows(), say, and that could destroy nextFrame right before we end up using it again.  Possibly similar issues at other callsites.

The script blocker needs to go as high up the stack as makes sense, and all code outside the scriptblocker needs to be aware that script can run under it.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Over IRC bz and I agreed that this is something that can wait for 3.5.1.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

The other frame destruction calls or frame creation calls all go through nsListBoxBodyFrame::OnContentInserted/Removed, which come from nsCSSFrameConstructor::ContentInserted/Removed, which should have scriptblockers, but I manually checked all cases with mxr anyways.

Should nsCSSFrameConstructor::BeginUpdate and/or AUTO_LAYOUT_PHASE_ENTRY_POINT for frame construction assert without a scriptblocker?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 380954 [details] [diff] [review]
patch v2

> Should nsCSSFrameConstructor::BeginUpdate and/or AUTO_LAYOUT_PHASE_ENTRY_POINT
> for frame construction assert without a scriptblocker?

Both might not be a bad idea.  Note that there are calls to creation that come via ReflowFinished, but that has a scriptblocker, so we're ok there.

This patch is still no good though.  For example, once the scriptblocker in DestroyRows goes out of scope, it might run script that destroys |this|.  Then your PresContext() call will crash.

Similar for ReverseDestroyRows() and DoInternalPositionChanged().  It might work to do a scriptblocker at the very beginning of DoInternalPositionChanged and audit callers; for example in that case ScrollToIndex is likely to need a weakframe check....  But please make sure to check exactly what code can run after scriptblockers go out of scope!

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: patch v3

The assert for scripts being blocked in nsCSSFrameConstructor::BeginUpdate was getting hit a lot and I don't want to look into it right now so I'm not including it.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 380994 [details] [diff] [review]
patch v3

Please file a followup on the BeginUpdate thing?

Good catch on the DoInternalPositionChanged code not being safe if one of the Run() calls kills the frame, but your new code is also unsafe: it will fail to revoke the events after that one, and when they end up running they'll dereference the dead frame.  You need to only run the event if IsAlive(), but revoke unconditionally.

I would also put the scriptblocker in DoInternalPositionChanged after the delta==0 early return.

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: patch v4

I also noticed that nsPositionChangedEvent::Run was confused; it tries to remove itself from its mFrame's mPendingPositionChangeEvents array, when the only time we run a nsPositionChangedEvent is after clearing mPendingPositionChangeEvents and putting the elements into a local array.

Comment 10

[Timothy Nikkel (:tnikkel)]

Followup bug 496020 filed for nsCSSFrameConstructor::BeginUpdate being called when scripts are not blocked.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 381161 [details] [diff] [review]
patch v4

Can we assert in Run() that the element is not in the array?  With that, looks good

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: patch v5

It was me who was confused, not nsPositionChangedEvent::Run. Removed that change.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/3d8bbe41b2f7

Comment 14

[Boris Zbarsky [:bzbarsky]]

I had to back out the nsPresContext.h change (http://hg.mozilla.org/mozilla-central/rev/b9ad86e82328) because it caused Windows build failures.  Specifically, TestWinTSF.cpp failed to compile.  It looks like this happened because it does some Really Weird Stuff with string headers and includes nsPresContext.h, and nsContentUtils.h happened to include string headers.  Some selected build errors:

c:\builds\mozilla\debug\obj-firefox\dist\include\nsReadableUtils.h(53) : error C
4430: missing type specifier - int assumed. Note: C++ does not support default-i
nt
c:\builds\mozilla\debug\obj-firefox\dist\include\nsReadableUtils.h(53) : error C
2143: syntax error : missing ',' before '<'
c:\builds\mozilla\debug\obj-firefox\dist\include\nsReadableUtils.h(55) : error C
2065: 'end' : undeclared identifier
c:\builds\mozilla\debug\obj-firefox\dist\include\nsReadableUtils.h(55) : error C
2228: left of '.get' must have class/struct/union
        type is ''unknown-type''
c:\builds\mozilla\debug\obj-firefox\dist\include\nsReadableUtils.h(55) : error C
2065: 'start' : undeclared identifier
c:\builds\mozilla\debug\obj-firefox\dist\include\nsReadableUtils.h(55) : error C
2228: left of '.get' must have class/struct/union
        type is ''unknown-type''
....
c:\builds\mozilla\debug\obj-firefox\dist\include\nsReadableUtils.h(62) : error C
2375: 'LossyCopyUTF16toASCII' : redefinition; different linkage
        c:\builds\mozilla\debug\obj-firefox\dist\include\nsStringAPI.h(942) : se
e declaration of 'LossyCopyUTF16toASCII'
c:\builds\mozilla\debug\obj-firefox\dist\include\nsReadableUtils.h(63) : error C
2375: 'CopyASCIItoUTF16' : redefinition; different linkage
        c:\builds\mozilla\debug\obj-firefox\dist\include\nsStringAPI.h(948) : se
e declaration of 'CopyASCIItoUTF16'

Comment 15

[Boris Zbarsky [:bzbarsky]]

I've extended the hackery in the test and repushed the prescontext change.  See http://hg.mozilla.org/mozilla-central/rev/7192582eb846

But that test is a landmine waiting to happen.  Can we fix it to not do what it does?

Comment 16

[Timothy Nikkel (:tnikkel)]

(In reply to comment #15)
> But that test is a landmine waiting to happen.  Can we fix it to not do what it
> does?

Filed bug 497812 for that.

Comment 17

[Daniel Veditz [:dveditz]]

This was marked wanted1.9.1.x+ -- do we still want it on the 1.9.1 branch?

Comment 18

[Timothy Nikkel (:tnikkel)]

Yes, I think so. This bug should be considered in concert with bug 498228, which fixes an annoying regression from this bug.