Closed Bug 812744 Opened 12 years ago Closed 12 years ago

Compartment mismatch with onevent attribute

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20
Tracking Flags:
Tracking Status
firefox18 --- unaffected
firefox19 + fixed
firefox20 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: jruderman, Assigned: bzbarsky)

References

Details

(Keywords: assertion, sec-critical, testcase, Whiteboard: [adv-main19-])

Attachments

(4 files, 3 obsolete files)

testcase
416 bytes, text/html
Details
stack
17.70 KB, text/plain
Details
We really do need the scope object, not the script handling object!
4.11 KB, patch
smaug
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review
XSLT patch with explicit boolean flag
14.38 KB, patch
sicking
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Attached file testcase — 
Assertion failure: false (compartment mismatched), at js/src/jscntxtinlines.h:204
Attached file stack — 

    
    

    
  
Probably a regression from bug 807226.
Blocks: 807226

    
    

    
  
Hrm.  So the issue here is that we're doing the setAttribute after the subframe has finished loading.  And we're doing it on the default compartment of the script context... but the script context is now attached to the new page that's been loaded.  So we get a handler compiled that's not same-compartment with the node it's on.

    
  

          tracking-firefox19:
          --- → ?

    
    

    
  
So in a build from before bug 807226, what happens is that we call the onmouseleave getter, go to compile the handler, compile it in the compartment of the new web page in the subframe (I think that's a pretty bad bug!) then go to call nsJSContext::BindCompiledEventHandler.  In this function we wrap the target (which is the about:blank <html> element), and we wrap it into the correct about:blank compartment, as expected.  So the bound function ends up in the right compartment and we don't get the assert.

Of course we still compiled the function in the wrong compartment, which is really really scary to me.  :(

    
    

    
  
So in a current build, at first things go the same way.  We compile an event handler, then we go to bind it.  At this point the aScope we're passing in is a proxy and the aHandler is compiled in the compartment of the new subframe page.  The proxy is also from that post-load subframe compartment, note.  And the reason for that is that in nsEventListenerManager::SetEventHandler (the one that takes a string) we got the scope from doc->GetScriptGlobalObject(), which claims to return the inner window... but will return the _outer_ window for unloaded documents.

In any case, we call the event handler constructor, and pass it the scope thing as aOwner.  But event handlers make sure to wrap their callable into the owner compartment, so we take our cloned-into-the-right-compartment function and wrap it into the aOwner compartment, which is not the compartment we want here.

So I guess the fact that we compile initially in the new-page compartment is not really necessarily a huge problem, since we never actually run that function, and binding clones into a different compartment, instead of just proxying.

But we should definitely stop using GetScriptGlobalObject here and use GetScopeObject instead, as far as I can tell.  I'll do that tomorrow.
Assignee: nobody → bzbarsky

    
    

    
  
OK, using the scope object makes us fail the test for bug 379120.  Presumably because the transformed document has a non-null scope object or something?  I'll poke at it a bit more...  Maybe the right answer is to get the script global, and if non-null _then_ use the scope object.

    
    

    
  


  
Please let me know if you want me to not check in the comments or the testcase for some reason.  I see no reason to, since this is a pure-trunk issue.

        Attachment #682815 -
        Flags: review?(bugs)

    
  
Whiteboard: [need review]

    
    

    
  
Comment on attachment 682815 [details] [diff] [review]
Use the document scope object, not the script global, to find the JSObject to use as a scope when creating event handlers.   option would be to just explicitly wrap the node and use that, for what it's worth.  That wouldn't give us an nsIScriptContext, th

># HG changeset patch
># User Boris Zbarsky <bzbarsky@mit.edu>
># Date 1353180302 28800
># Node ID a8b024b7df554daacd5d64f0230f51a72008e605
># Parent  750cb3a1d08fdb3e71a5c2c52399f09a3a384a96
>Bug 812744.  Use the document scope object, not the script global, to find the JSObject to use as a scope when creating event handlers.  r=smaug
>
>Anoother option would be to just explicitly wrap the node and use that, for what it's worth.  That wouldn't give us an nsIScriptContext, though.
>
>diff --git a/content/events/src/nsEventListenerManager.cpp b/content/events/src/nsEventListenerManager.cpp
>--- a/content/events/src/nsEventListenerManager.cpp
>+++ b/content/events/src/nsEventListenerManager.cpp
>@@ -599,17 +599,42 @@ nsEventListenerManager::SetEventHandler(
> 
>   nsCOMPtr<nsIScriptGlobalObject> global;
> 
>   if (node) {
>     // Try to get context from doc
>     // XXX sXBL/XBL2 issue -- do we really want the owner here?  What
>     // if that's the XBL document?
>     doc = node->OwnerDoc();
>-    global = doc->GetScriptGlobalObject();
>+    // This is all a bit of a mess:
>+    // 1)  GetScriptGlobalObject usually returns the inner window, but after a
>+    //     document has been unloaded it starts returning the outer window(!!.
>+    // 2)  GetScriptHandlingObject will return the window of some random other
>+    //     document for data documents (XHR, XSLT result documents).
Why random? XHR should pass its GetOwner() as the scripthandling object to the document.
IIRC also XSLT



>+    // 3)  GetScopeObject will do the same as GetScriptHandlingObject, but
>+    //     return null for non-data documents that have been unloaded.
>+    // 4)  GetInnerWindow returns exactly the same thing as
>+    //     GetScriptGlobalObject once mRemovedFromDocShell becomes true.  In
>+    //     particular, it starts returning the outer window(!!).
True, IIRC I just changed something to be inline in common case but didn't change the behavior that
mrbkap was fixing.


>+    // So the upshot is there is no way to get the real inner window... and even
>+    // no good way to know that you can't get it.  Also, this is all way
>+    // over-complicated.  :(
What is wrong with GetScriptHandlingObject?

    
    

    
  
Comment on attachment 682815 [details] [diff] [review]
Use the document scope object, not the script global, to find the JSObject to use as a scope when creating event handlers.   option would be to just explicitly wrap the node and use that, for what it's worth.  That wouldn't give us an nsIScriptContext, th

r- because I don't understand what is wrong with GetScriptHandlingObject

        Attachment #682815 -
        Flags: review?(bugs) → review-

    
    

    
  
> Why random? XHR should pass its GetOwner() as the scripthandling object to the document.

From the point of view of this code, I believe that's equivalent to "random"....

> What is wrong with GetScriptHandlingObject?

It makes us compile onfoo attributes in XHR documents and XSLT result documents.  In addition to failing our test suite (see comment 7), that just seems wrong.  It'd definitely freak out web developers to see syntax errors for event handlers in XHR result documents suddenly appear in their error consoles!

    
    

    
  
GetScriptHandlingObject should work here.
In case of non-browsing-context-documents it uses the slow path nsDocument::GetScriptHandlingObjectInternal()
That shouldn't return anything random.

    
    

    
  
OK.  In that case, I need to flag XSLT result documents as data documents.  I just tested Safari and Chrome, and in both the handler is not compiled.  It _is_ compiled in Opera, but Opera also compiles event handlers in XHR result documents, so I'm not going to worry about its behavior here.  IE doesn't seem to expose the same API for scripted XSLT, so no idea whether that's even possible there.

    
    

    
  
Oh, so I thought event handlers should be compiled (to not regress existing Gecko behavior) in
XSLT documents. But ok, I see GetScriptGlobalObject returns null for XSLT docs.
In that case making them data documents sounds ok.

    
  

        Attachment #682815 -
        Attachment is obsolete: true

    
  

        Attachment #683446 -
        Attachment is obsolete: true

        Attachment #683446 -
        Flags: review?(bugs)

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #17)
> Created attachment 683460 [details] [diff] [review]
> We really do need the scope object, not the script handling object!
Why? We shouldn't be handling events in unloaded documents (at least not with the current
setup where we do inner window checks.)

    
    

    
  
We shouldn't be _handling_ events, but we should still be compiling the handlers, imo, both to keep compat with what we do now and per spec.

Note that a document can go from unloaded to loaded with bfcache...

    
    

    
  
Comment on attachment 683460 [details] [diff] [review]
We really do need the scope object, not the script handling object!

Ok. I'm not quite happy with this setup but who can.
Need to sort out the scriptglobal/scopeobject/scripthandlingobject mess.

        Attachment #683460 -
        Flags: review?(bugs) → review+

    
    

    
  
The!! aResult test here isn't that great. It might very well break in the future. Could you pass an explicit flag to TransformToDoc instead?

    
    

    
  


  
Jonas, here you go!

        Attachment #683896 -
        Flags: review?(jonas)

    
  

        Attachment #683445 -
        Attachment is obsolete: true

        Attachment #683445 -
        Flags: review?(jonas)

    
    

    
  
Comment on attachment 683896 [details] [diff] [review]
XSLT patch with explicit boolean flag

Review of attachment 683896 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

        Attachment #683896 -
        Flags: review?(jonas) → review+

    
    

    
  
Comment on attachment 683460 [details] [diff] [review]
We really do need the scope object, not the script handling object!

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 807226, sort of
User impact if declined: Compartment mismatches, possible security bugs
Testing completed (on m-c, etc.): Passes automated tests
Risk to taking this patch (and alternatives if risky): Risk is medium.  I'm not
   sure there are great alternatives, unfortunately.
String or UUID changes made by this patch: None

        Attachment #683460 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 683896 [details] [diff] [review]
XSLT patch with explicit boolean flag

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Needed so that the patch that fixes
   this bug doesn't regress tests.
User impact if declined: Same as main patch.
Testing completed (on m-c, etc.): Same as main patch.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None.

        Attachment #683896 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Sorry, something in your push turned the mochitests orange. I had to back the entire thing out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/461a2225b7ba
Target Milestone: mozilla20 → ---

    
  

        Attachment #683460 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

    
    

    
  
Comment on attachment 683896 [details] [diff] [review]
XSLT patch with explicit boolean flag

Waiting for the regression fix before taking on Aurora.

        Attachment #683896 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

    
  

        Attachment #683896 -
        Flags: approval-mozilla-aurora- → approval-mozilla-aurora?

    
  

        Attachment #683460 -
        Flags: approval-mozilla-aurora- → approval-mozilla-aurora?

    
    

    
  
Comment on attachment 683460 [details] [diff] [review]
We really do need the scope object, not the script handling object!

Fixing a medium risk sec-critical regression while it's still on Aurora is righteous. If there are specific things that QA can test manually, please add qawanted/verifyme here along with a description of requested testing.

        Attachment #683460 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
  

        Attachment #683896 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [adv-main19-]
Group: core-security

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: