Bugzilla@Mozilla – Bug 432591
Fix for bug 428672 can be circumvented by using XUL element
Last modified: 2008-07-02 02:00:06 PDT
Summon comment box
Instead of <body> element, a XUL element can be used to attach an event handler to an outer window.
Created attachment 319837 [details] [review] like so?
This seems like a route to whack-mole. Can we fix nsDocument::GetInnerWindow to return null instead of an outer window? I started looking through callers (of which there are a lot fewer than nsDocment::GetScriptGlobalObject) and it seems like all of them would be happy with such a change.
Created attachment 319840 [details] [review] Fix (same as for bug 428672).
Created attachment 319842 [details] [review] less mole-whacking This does fix the bug... we can fix null-derefs whack-a-mole style (and easily), and at least it's not an XSS vulnerability.
Comment on attachment 319842 [details] [review] less mole-whacking Sorry, didn't see your original patch before I attached mine :( We do want to do this, but not now IMO. The regression risk from this is unfortunately pretty high (as we've witnessed before), so we should do this as part of 431767 early in a release instead of now.
Comment on attachment 319840 [details] [review] Fix (same as for bug 428672). a1.9+=damons
jst, so I understand, you're advocating for your patch now, and crowder's patch as a 1.9.0.x thing? If so, can we get a new bug filed on that with crowder's patch attached so we can continue to track that? thx.
Beltzner, yes, bug 431767 already on file. Fix checked in.
Do we need a different patch for 1.8?
Created attachment 320617 [details] [review] 1.8 branch version (tested, works). This is the same patch for 1.8 (for this bug and for bug 428672 as well), though it's untested (I'm unable to build 1.8 due to cairo version dependency issues it seems). If someone is able to test this I'd appreciate it.
Comment on attachment 320617 [details] [review] 1.8 branch version (tested, works). Flagging dveditz for review of this so he can test. bsterne might also be able to.
Comment on attachment 320617 [details] [review] 1.8 branch version (tested, works). I just tested this fix, and it fixes the problem reported in this bug.
Comment on attachment 320617 [details] [review] 1.8 branch version (tested, works). r=dveditz Approved for 1.8.1.15, a=dveditz for release-drivers That's two pretty similar bugs -- do we need to audit all the calls to GetScriptGlobalObject() and GetInnerWindow() and make sure they don't have this problem?
Fix checked in. Dan, yes, we're spot fixing here. There's a bug on file already to make GetInnerWindow() (on trunk) always really only return an inner window, but that's not something I'd be willing to land on the branch until it's gotten a fair bit of trunk testing.
Verified for branch 2.0.0.15 release with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre) Gecko/2008061005 BonEcho/2.0.0.15pre for testcase 1.