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
337 bytes, text/html
Details
testcase where the exception "escapes" to the console
365 bytes, text/html
Details
Fix the "did the DOM call throw?" test in IonMonkey to check the return value correctly.
3.09 KB, patch
jandem
: review+
lsblakk
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Beta patch
7.22 KB, 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 —
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 — — 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: