Closed Bug 847119 Opened 12 years ago Closed 12 years ago

DOM exception is not caught properly

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22
Tracking Flags:
Tracking Status
firefox20 --- fixed
firefox21 --- fixed
firefox22 --- fixed

People

(Reporter: jruderman, Assigned: bzbarsky)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [fuzzblocker])

Attachments

(4 files)

testcase that asserts
(deleted), text/html
Details
testcase where the exception "escapes" to the console
(deleted), text/html
Details
Fix the "did the DOM call throw?" test in IonMonkey to check the return value correctly.
(deleted), patch
jandem
: review+
lsblakk
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Beta patch
(deleted), patch
jandem
: review+
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review 
With:

  user_pref("javascript.options.ion.parallel_compilation", false);
  user_pref("javascript.options.methodjit.content", false);

And a build compiled with:

  --enable-debug
  --enable-optimize (!!)

Various bad things happening after ion-compiling the function:

  function domthrows() { document.documentElement.appendChild(null); }

Regression from bug 839116 part 2?
Attached file testcase that asserts (deleted) — 
Assertion failure: !JS_IsExceptionPending(cx), at js/src/jsiter.cpp:737

In js_ThrowStopIteration

    
    

    
  
Our Tinderbox debug builds are built with --enable-optimize, so you should be able to reproduce the bug using e.g.

https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx64-debug/1362259729/

    
    

    
  
> Regression from bug 839116 part 2?

For appendChild on the <html>, perhaps.  What about:

  var xhr = new XMLHttpRequest();
  function domthrows() { xhr.open();  }

?  I don't see what the DOM code could possibly be doing to make this complicated for the jit per se, except if the specialized Ion codepath for DOM calls is busted.

    
    

    
  
So as expected, turning off the code bug 773549 added makes this problem go away.

I checked, and we never take the target->jitInfo()->isInfallible branch in CodeGenerator::visitCallDOMNative, which is not surprising.  So we are in fact generating the exception-handling code the other branch puts in....

Here's what JIT Inspector has to say about domthrows() when I write it per comment 4:

[CallDOMNative]
... set up the call ...
call       *%rax
addq       $0x8, %rsp
pop        %rsp
testl      %eax, %eax
je         ((279))
movq       0x30(%rsp), %rcx
jmp        ((289))
##link     ((279)) jumps to ((289))
subq       $0x8, %rsp
movq       %rsp, %rax
movq       %rsp, %rcx
andq       $0xfffffff0, %rsp
push       %rcx
subq       $0x8, %rsp
movq       %rax, %rdi
movl       %esp, %eax
testq      $0xf, %rax
je         ((326))
int3
##link     ((326)) jumps to ((327))
movabsq    $0x101446870, %rax
call       *%rax
addq       $0x8, %rsp
pop        %rsp
movabsq    $0xfffa00000000000f, %rcx
movq       0x0(%rsp), %rsp
ret
##link     ((289)) jumps to ((359))
addq       $0x38, %rsp

and then we have the [TypeBarrier].

Sean, does that (and the code in CodeGenerator::visitCallDOMNative in general) look obviously wrong in some way?

    
    

    
  
And actually requesting needinfo from Sean... ;)
Flags: needinfo?(sstangl)

    
  
Blocks: 773549
No longer blocks: 839116

    
  
Keywords: regression

    
  
Assignee: nobody → bzbarsky

    
    

    
  
Comment on attachment 721285 [details] [diff] [review]
Fix the "did the DOM call throw?" test in IonMonkey to check the return value correctly.

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

Don't we have to fix GetDOMProperty and SetDOMProperty too, since they have a similar check? r=me with that addressed.

        Attachment #721285 -
        Flags: review?(jdemooij) → review+

    
    

    
  
> Don't we have to fix GetDOMProperty and SetDOMProperty too

Nice catch.  I can't come up with a testcase that fails for those, but of course whether we fail depends on what's in the register....

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/da51c5373da5

Jan, do you think we should backport this to branches?
Flags: needinfo?(sstangl)
Flags: needinfo?(jdemooij)
Flags: in-testsuite+
Target Milestone: --- → mozilla22

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #10)
> Jan, do you think we should backport this to branches?

Not sure how critical/common this issue is, but it would be good to backport imo. The fix is simple and we use similar code elsewhere. The main problem I see is that there's no branchTestBool on beta (aurora has it though), but should be easy to backport that, just a few lines of code.
Flags: needinfo?(jdemooij)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Beta patch (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #721383 -
        Flags: review?(jdemooij)

    
    

    
  
Comment on attachment 721285 [details] [diff] [review]
Fix the "did the DOM call throw?" test in IonMonkey to check the return value correctly.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 773549
User impact if declined: Possible incorrect behavior for some websites; more
   easily hit now that more objects are on WebIDL bindings.
Testing completed (on m-c, etc.): Passes attached test.
Risk to taking this patch (and alternatives if risky): Risk should be quite low.
String or UUID changes made by this patch: None.

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

    
  

        Attachment #721383 -
        Flags: approval-mozilla-beta?

        Attachment #721383 -
        Flags: review?(jdemooij) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/da51c5373da5
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

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

    
    

    
  
Comment on attachment 721383 [details] [diff] [review]
Beta patch

Looks good, let's get this in soon so we have some bake time in beta 4/5 to make sure there's no unexpected fallout before shipping.

        Attachment #721383 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+
Component: DOM → DOM: Core & HTML

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: