742222 - Execution of request.onreadystatechange crashes Firefox

Status: VERIFIED FIXED

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

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Firebug test build

This report is based on the following thread:
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/49c2d30c38c40504/81252a5b7b76b655?hl=en#81252a5b7b76b655


Firebug is overriding <xhr-request>.onreadystatechange and when the
callback is executed it calls the original handler, see here: 

https://github.com/firebug/firebug/blob/572bc4883aa895b2bcac53e555e6f6434aa11ec1/extension/content/firebug/net/spy.js#L819

onreadystatechange is now just a Function (since Fx14) so, it's executed using 'call':

var originalHandler = request.onreadystatechange;
originalHandler.call(request, event);   // <- causes crash

Execution of the second line causes Firefox crash.

Crash Report:
https://crash-stats.mozilla.com/report/index/bp-2fcfa090-a26b-43d6-bbcc-45aef2120403

The first Nightly where the bug appeared:
http://hg.mozilla.org/mozilla-central/rev/3e46009daea3

It still appears in today's Nightly:
http://hg.mozilla.org/mozilla-central/rev/c410b2d6d570

STR:

1) Install attached Firebug build
2) Enable the Console panel
3) Load www.google.com
4) Type something into the search box -> CRASH

Honza

Comment 1

[Boris Zbarsky [:bzbarsky]]

Peter, are we possibly ending up returning the content function to chrome code without putting a cross-compartment wrapper on it?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yup.  In a debug build, I get:

Assertion failure: compartment mismatched, at ../../../mozilla/js/src/jscntxtinlines.h:156

(gdb) frame 7
#7  0x135c8876 in js::CallJSNative (cx=0x22c529d0, native=0x16179ec0 <get_onreadystatechange>, args=@0xbfff65b4) at jscntxtinlines.h:316
316             assertSameCompartment(cx, args.rval());

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure to JS_WrapValue values being returned for callback types and 'any' types.

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Perhaps this one is related?
Bug 742631 - Crash [@ js::ContextStack::currentScriptWithDiagnostics(unsigned char**)] 

Honza

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 612189 [details] [diff] [review]
Make sure to JS_WrapValue values being returned for callback types and 'any' types.

Would be nice to have a test.

Comment 6

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/mozilla-central/rev/1907c8bf3cae with test.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Er, https://hg.mozilla.org/integration/mozilla-inbound/rev/1907c8bf3cae

Comment 8

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/1907c8bf3cae

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

My STR from comment #0 is fixed in
http://hg.mozilla.org/mozilla-central/rev/9ca66ce2672f

Thanks!

Honza