Closed
Bug 495728
Opened 16 years ago
Closed 16 years ago
###!!! ASSERTION: destroy called on frame while scripts not blocked: '!nsContentUtils::IsSafeToRunScript()', file /Users/bzbarsky/mozilla/vanilla/mozilla/layout/generic/nsFrame.cpp, line 446
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
Tracking | Status | |
---|---|---|
status1.9.1 | --- | ? |
People
(Reporter: bzbarsky, Assigned: tnikkel)
References
Details
(Keywords: regression)
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•16 years ago
|
Flags: blocking1.9.1?
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → tnikkel
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 1•16 years ago
|
||
I think bz was right, we just need a script blocker here.
Attachment #380794 -
Flags: review?(bzbarsky)
Reporter | ||
Updated•16 years ago
|
Attachment #380794 -
Flags: review?(bzbarsky) → review-
Reporter | ||
Comment 2•16 years ago
|
||
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•16 years ago
|
||
Over IRC bz and I agreed that this is something that can wait for 3.5.1.
Flags: wanted1.9.1.x+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Assignee | ||
Comment 4•16 years ago
|
||
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?
Attachment #380794 -
Attachment is obsolete: true
Attachment #380954 -
Flags: review?(bzbarsky)
Reporter | ||
Updated•16 years ago
|
Attachment #380954 -
Flags: review?(bzbarsky) → review-
Reporter | ||
Comment 5•16 years ago
|
||
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!
Assignee | ||
Comment 7•16 years ago
|
||
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.
Attachment #380954 -
Attachment is obsolete: true
Attachment #380994 -
Flags: review?(bzbarsky)
Reporter | ||
Updated•16 years ago
|
Attachment #380994 -
Flags: review?(bzbarsky) → review-
Reporter | ||
Comment 8•16 years ago
|
||
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.
Assignee | ||
Comment 9•16 years ago
|
||
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.
Attachment #380994 -
Attachment is obsolete: true
Attachment #381161 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 10•16 years ago
|
||
Followup bug 496020 filed for nsCSSFrameConstructor::BeginUpdate being called when scripts are not blocked.
Reporter | ||
Updated•16 years ago
|
Attachment #381161 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 11•16 years ago
|
||
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
Assignee | ||
Comment 12•16 years ago
|
||
It was me who was confused, not nsPositionChangedEvent::Run. Removed that change.
Attachment #381161 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Attachment #381905 -
Flags: superreview?(bzbarsky)
Reporter | ||
Updated•16 years ago
|
Attachment #381905 -
Flags: superreview?(bzbarsky) → superreview+
Reporter | ||
Updated•16 years ago
|
Attachment #381905 -
Flags: review+
Reporter | ||
Comment 13•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Reporter | ||
Comment 14•16 years ago
|
||
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'
Reporter | ||
Comment 15•16 years ago
|
||
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?
Assignee | ||
Comment 16•16 years ago
|
||
(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•15 years ago
|
||
This was marked wanted1.9.1.x+ -- do we still want it on the 1.9.1 branch?
status1.9.1:
--- → ?
Assignee | ||
Comment 18•15 years ago
|
||
Yes, I think so. This bug should be considered in concert with bug 498228, which fixes an annoying regression from this bug.
Updated•15 years ago
|
Target Milestone: --- → mozilla1.9.2a1
You need to log in
before you can comment on or make changes to this bug.
Description
•