We can get into situations where an element's box object becomes useless because of some sequence of events like this: frames are destroyed (e.g. in response to opacity changes (?)), InvalidatePresentationStuff is called on the box object in question, but the document still holds it in its boxobject table, so that when script attempts to subsequently access boxobject methods, the busted boxobject with null mPresShell etc is used, and none of the methods work. This was noted in earlier testing versions of my new prefwindow where I had a listbox and was attempting to access properties on it after the pane was faded in. The solution appears to be that in some cases when InvalidatePresentationStuff is called, we want to tell the document to take it out of the boxobject table, which is what this patch does. See also the patch in 282103, which updates the call sites in nsXULDocument.

*** Bug 282104 has been marked as a duplicate of this bug. ***

Comment on attachment 174197 [details] [diff] [review] patch So... the only caller with PR_TRUE will be nsListBoxBodyFrame::Destroy, right? In that case, may I ask why this is nuking the box object in the first place? It seems to me like the listbox is just abusing the InvalidatePresentationStuff() api here; while it should be setting the listboxbody property to null when the listbox frame is destroyed, it should NOT be nulling out the presshell. It seems that it wants a separate method it can call on the nsListBoxObject that will do that.

No longer blocking anything.

Might I ask why not? I can see that it might not be blocking Firefox 1.1, but why isn't it blocking toolkit/gecko 1.8?

This is causing toolkit users problems, apparently. See bug 322513. I do think we should fix this...

OK, this also causes a crash that's most likely an exploitable security hole (bug 325045). Further, in debugging that I figured out that the way this is set up means that the 1.7 branch is also exploitable in the same way (since content can call setPropertyAsSupports). I guess I'll take this and see what I can do here. :( Does rather make me wish someone had answered my question from comment 5....

So what these patches do is make sure that content JS 1) Can't get hold of the list box body frame via XPConnect. 2) Can't mess with our boxobject's internal frame pointers. Both are accomplished by having some nonscriptable private interfaces.

Comment on attachment 210093 [details] [diff] [review] This should do the trick. Testing now.... >+ // listboxBody is always null. It's only here to avoid changing the >+ // interface. Surely we can just this completely on trunk? >+ "Calling SetPropertyAsSupports on a frame. Prepare to crash " >+ "and be exploited any time some random website decides to " >+ "exploit you"); Repetition of /\bexploit/ :-P I think "and be exploited" is obvious given the following text. >- SetPropertyAsSupports(NS_LITERAL_STRING("listboxbody").get(), nsnull); >+ ClearCachedListBoxBody(); Using a virtual call to set a member variable?

Comment on attachment 210109 [details] [diff] [review] Same for 1.8 branch Some previous comments may be relevant.

> Repetition of /\bexploit/ :-P I think "and be exploited" is obvious given the > following text. The following text doesn't make sense without the last part, unless I'm missing something. Care to suggest the wording you meant? > Using a virtual call to set a member variable? Yep. It's cleaner in this case, and this is not perf-intensive code.

Oh, missed one comment: > Surely we can just this completely on trunk? I'd love to do that as a separate patch, but I wanted to minimize merging as much as possible. As it was, it took me a good long while to get this stuff merged...

BZ please land on trunk!

...when you have the reviews you need.

Comment on attachment 210093 [details] [diff] [review] This should do the trick. Testing now.... > NS_IMETHODIMP > nsListBoxObject::GetListboxBody(nsIListBoxObject * *aListboxBody) > { >- *aListboxBody = GetListBoxBody(); >- NS_IF_ADDREF(*aListboxBody); >+ *aListboxBody = nsnull; > return NS_OK; > } Why does this return null now?

One of these days I'll learn to read scrollback. Ignore my previous question.

Comment on attachment 210093 [details] [diff] [review] This should do the trick. Testing now.... sr=ben@mozilla.org

Comment on attachment 210109 [details] [diff] [review] Same for 1.8 branch sr=ben@mozilla.org

Comment on attachment 210110 [details] [diff] [review] Same for 1.7/aviary branches sr=ben@mozilla.org

Fixed on trunk.

This checkin caused the following on my Win32 cygwin MingW Thunderbird build :- e:/mozilla/source/mozilla/layout/xul/base/src/nsListBoxObject.cpp:87: error: ext ra `;' I think it actually means line 70 (as that is what my Linux build complains about).

jst fixed the build bustage and I'll make sure to fix it in the branch patches before landing. For future reference, please just comment; don't reopen. That doesn't send _nearly_ as much bugspam.

Fixed on 1.8.1 branch.

Comment on attachment 210109 [details] [diff] [review] Same for 1.8 branch a=dveditz for drivers. Please land soon so we can shake this out.

Comment on attachment 210110 [details] [diff] [review] Same for 1.7/aviary branches a=dveditz for drivers, ditto

Fixed on MOZILLA_1_8_0_BRANCH, MOZILLA_1_7_BRANCH, AVIARY_1_0_1_20050124_BRANCH.

Boris: If you could provide a quick way for QA to verify the fix in the bug for 1.0.8/1.7.13, that would be very helpful.

Boris: Ignore my last comment, please. We won't get to verifying some of the non-critical bugs for this release. (In reply to comment #29) > Boris: If you could provide a quick way for QA to verify the fix in the bug for > 1.0.8/1.7.13, that would be very helpful. >

Marcia, the bugs this bug blocks have testcases that should be verified (that's bug 322513 and bug 325045). Note that one of them is critical but just didn't have the fixed-* keywords set, since they were set on this bug (which covered the main problem). I'll try to set fixed-* keywords on all bugs a checkin fixes in the future, not just the bug where the work is happening.

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060224 Firefox/1.5.0.1, based on verifications of bug 322513 and bug 325045 (pre bz in comment #31)