733793 - Crash in js::CrossCompartmentWrapper::getPropertyDescriptor @ xpc::WrapperFactory::PrepareForWrapping (mainly correlated to Youtube Video Replay)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Scoobidiver (away)]

It's a new crash signature that first appeared in 13.0a1/20120223.
The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=373c710112e6&tochange=5e756e59a794
It might be a regression from bug 717113.

Signature 	xpc::WrapperFactory::PrepareForWrapping(JSContext*, JSObject*, JSObject*, unsigned int) More Reports Search
UUID	f38d49ed-58b1-4b0d-a0c4-3210c2120307
Date Processed	2012-03-07 13:46:46
Uptime	5
Last Crash	28.9 minutes before submission
Install Age	29.2 minutes since version was first installed.
Install Time	2012-03-07 14:30:34
Product	Firefox
Version	13.0a1
Build ID	20120306031101
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7600
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 23 stepping 10
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x0
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x2a42, AdapterSubsysID: 030d1025, AdapterDriverVersion: 8.15.10.2202
D2D? D2D+ DWrite? DWrite+ 
Processor Notes 	This dump is too long and has triggered the automatic truncation routine
EMCheckCompatibility	True
Total Virtual Memory	2147352576
Available Virtual Memory	1897250816
System Memory Use Percentage	42
Available Page File	3201490944
Available Physical Memory	1200078848

Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	xpc::WrapperFactory::PrepareForWrapping 	js/xpconnect/wrappers/WrapperFactory.cpp:177
1 	mozjs.dll 	JSCompartment::wrap 	js/src/jscompartment.cpp:227
2 	mozjs.dll 	JSCompartment::wrap 	js/src/jscompartment.cpp:349
3 	mozjs.dll 	JSCompartment::wrap 	js/src/jscompartment.cpp:392
4 	mozjs.dll 	js::CrossCompartmentWrapper::getPropertyDescriptor 	js/src/jswrapper.cpp:543
5 	mozjs.dll 	js::Proxy::getPropertyDescriptor 	js/src/jsproxy.cpp:765
6 	mozjs.dll 	js_SetPropertyHelper 	js/src/jsobj.cpp:5380
7 	mozjs.dll 	js::Wrapper::set 	js/src/jswrapper.cpp:234
8 	mozjs.dll 	js::CrossCompartmentWrapper::set 	js/src/jswrapper.cpp:630
9 	mozjs.dll 	js::Proxy::set 	js/src/jsproxy.cpp:885
10 	mozjs.dll 	proxy_SetProperty 	js/src/jsproxy.cpp:1122
11 	mozjs.dll 	JSObject::nonNativeSetProperty 	js/src/jsobj.cpp:3102
12 	mozjs.dll 	js::SetPropertyOperation 	js/src/jsinterpinlines.h:372
13 	mozjs.dll 	js::mjit::stubs::SetName<0> 	js/src/methodjit/StubCalls.cpp:106
14 	mozjs.dll 	js::mjit::EnterMethodJIT 	js/src/methodjit/MethodJIT.cpp:1052
15 	mozjs.dll 	js::mjit::JaegerShot 	js/src/methodjit/MethodJIT.cpp:1123
16 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:451
17 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:517
18 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:549
19 	mozjs.dll 	js::ProxyHandler::call 	js/src/jsproxy.cpp:334
20 	mozjs.dll 	js::CrossCompartmentWrapper::call 	js/src/jswrapper.cpp:736
21 	mozjs.dll 	js::Proxy::call 	js/src/jsproxy.cpp:909
22 	mozjs.dll 	proxy_Call 	js/src/jsproxy.cpp:1428
23 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:492
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=xpc%3A%3AWrapperFactory%3A%3APrepareForWrapping%28JSContext*%2C%20JSObject*%2C%20JSObject*%2C%20unsigned%20int%29
https://crash-stats.mozilla.com/report/list?signature=xpc%3A%3AWrapperFactory%3A%3APrepareForWrapping

Comment 1

[Bobby Holley (:bholley)]

(In reply to Scoobidiver from comment #0)
> It's a new crash signature that first appeared in 13.0a1/20120223.
> It might be a regression from bug 717113.

Not super likely.

It looks like it's crashing here: http://hg.mozilla.org/mozilla-central/annotate/7d0d1108a14e/js/xpconnect/wrappers/WrapperFactory.cpp#l177

Hard to say for sure, but there's a good chance obj is null.

Comment 2

[Scoobidiver (away)]

It's currently #1 top crasher in 13.0a2.

One comment says: "Add On Incompatibility - Appearance"

Here are correlations per add-on on March 20:
  xpc::WrapperFactory::PrepareForWrapping(JSContext*, JSObject*, JSObject*, unsigned int)|EXCEPTION_ACCESS_VIOLATION_READ (30 crashes)
     87% (26/30) vs.   7% (27/394) {e1aaa9f8-4500-47f1-9a0a-b02bd60e4076} (Youtube Video Replay)
          3% (1/30) vs.   0% (1/394) 0.5
         83% (25/30) vs.   7% (26/394) 1.2

  xpc::WrapperFactory::PrepareForWrapping|EXC_BAD_ACCESS / KERN_INVALID_ADDRESS (20 crashes)
     75% (15/20) vs.  32% (15/47) {c75a27d8-4529-449f-b67b-aba65d7a1c0a} (Toggle Web Developer Toolbar 3.4)
     60% (12/20) vs.  26% (12/47) [e-mail] (RS+MU Downloader 2.4.1)
     60% (12/20) vs.  26% (12/47) [e-mail] (JavaScript Deobfuscator, https://addons.mozilla.org/addon/10345) (1.6.3)
     55% (11/20) vs.  23% (11/47) {dc572301-7619-498c-a57d-39143191b318} (Tab Mix Plus, https://addons.mozilla.org/addon/1122) (0.4.0)
     55% (11/20) vs.  23% (11/47) [e-mail] (Tilt 1.0.1)

I installed Youtube Video Replay 1.2 in 12.0b1, then I opened this profile with 13.0a2/20120312 and I got a "InternalError: too much recursion" prompt on first startup and bp-778df487-b91e-4fbf-afd1-d06a72120321 on second startup.

Comment 3

[Luke Wagner [:luke]]

It looks like the crash rate has a distinct downward trend.  Could a fix have been landed?

Comment 4

[Luke Wagner [:luke]]

So I looked a little more.  The callstacks definitely seem to be showing an infinite C-stack recursion.  Now, JSCompartment::wrap contains JS_CHECK_RECURSION, so blowing the stack probably isn't the cause.  Rather, some code that trips JS_CHECK_RECURSION, triggers an error, and then returns NULL.  Looking at the crash location:

  http://hg.mozilla.org/releases/mozilla-aurora/annotate/1eb6d3a4018b/js/xpconnect/wrappers/WrapperFactory.cpp#l177

I see a call to the outerObject hook (which can return null (see XPC_WN_OuterObject)) which doesn't check the return value for null.  (This is all in GetCurrentOuter, which I assume is inlined.)

Comment 5

[Scoobidiver (away)]

(In reply to Luke Wagner [:luke] from comment #3)
> It looks like the crash rate has a distinct downward trend.  Could a fix
> have been landed?
No. 76% of crashes happen within one minute.

Comment 6

[Luke Wagner [:luke]]

(In reply to Scoobidiver from comment #5)
> No. 76% of crashes happen within one minute.

I was looking at this graph: http://bit.ly/GVF7PH.  "No" doesn't seem to follow from "76% of crashes happen within one minute".  Regardless, we should fix the bug in comment 4.

Comment 7

[Scoobidiver (away)]

If a user crashes on startup, he won't use Firefox, so once all users that should crash hit it, there are no crashes, but it doesn't mean it's fixed.

Comment 8

[Luke Wagner [:luke]]

Ah, that makes more sense.

Comment 9

[Jorge Villalobos [:jorgev] (he/him)]

Adding Youtube Video Replay developer to CC list.

Comment 10

[David Mandelin [:dmandelin]]

What are the next actions on this bug?

 - Luke, can you write a patch for the bug in comment 4? Do you think that's the cause of these crashes?
 - Can we learn anything more from the stacks? There are some things that don't make sense to me, but it looks like some kind of property-set operation is going to a proxy, which triggers the same property-set operation?
   - Baris, does that sound familiar at all? Are you using proxies, or is it something internal to Firefox? Speaking of which, do you ever get this crash in testing?
   - Luke, would it make sense to add a diagnostic to check for this recursive operation to make it fail faster with a better error message?

Comment 11

[Luke Wagner [:luke]]

(In reply to David Mandelin from comment #10)
>  - Luke, can you write a patch for the bug in comment 4?

Sure, I was waiting for an xpconnect person to comment on this, but I guess it's pretty simple.

> Do you think that's the cause of these crashes?

The return value (NULL) and source location both match the crash address.

>    - Luke, would it make sense to add a diagnostic to check for this
> recursive operation to make it fail faster with a better error message?

Well, that's what JS_CHECK_RECURSION is.  Assuming xpconnect doesn't crash, you get the whole callstack in the exception.

Comment 12

[Luke Wagner [:luke]]

Attachment: null check

Comment 13

[Luke Wagner [:luke]]

Attachment: null check x2

Actually two are needed.

Comment 14

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed31daf07bd

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 610350 [details] [diff] [review]
null check x2

[Approval Request Comment]
Risk to taking this patch (and alternatives if risky): low, null check

Comment 16

[Baris Derin]

- David, I am not good at Firefox Internals but to me it somehow relates to AddonsManager and mozIJSSubScriptLoader.loadSubScript

Comment 17

[Baris Derin]

- David, I am not good at Firefox Internals but to me it somehow relates to AddonsManager and mozIJSSubScriptLoader. 

When trying the attached YVR.withAddonsManager.xpi, the line 37 in youtubevideoreplay_bootstrap.js:

   AddonManager.getAddonByID(YoutubeVideoReplay.addonGUID, function(addon) {  

was creating the Error Prompt. 

When trying the attached YVR.noAddonsManager.xpi which removed the AddonManager line the error is not thrown anymore.

Now and then, I see the below warning in Error Console:

Warning: anonymous function does not always return a value
Source file: resource://app/modules/XPIProvider.jsm -> file:///Users/barisderin/Library/Application%20Support/Firefox/Profiles/7gy2pfh7.YVR%201/extensions/%7Be1aaa9f8-4500-47f1-9a0a-b02bd60e4076%7D/bootstrap.js -> file:///Users/barisderin/Library/Application%20Support/Firefox/Profiles/7gy2pfh7.YVR%201/extensions/%7Be1aaa9f8-4500-47f1-9a0a-b02bd60e4076%7D/chrome/content/youtubevideoreplay_bootstrap.js
Line: 45

- Jorge, to protect FF 13 users I am going to remove Bootstrap support and switch to Restart version for the add-on till the bug is fixed. Is it OK?

Comment 18

[Baris Derin]

Attachment: Youtube Video Replay with Addons Manager

Comment 19

[Baris Derin]

Attachment: Youtube Video Replay No Addons Manager

Comment 20

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/7ed31daf07bd

Comment 21

[Luke Wagner [:luke]]

Oops, I should have added [leave open after merge].  I'll reopen so we can continue to track the issue and see if the patch fixes the topcrash.

Comment 22

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Baris Derin from comment #17)
> - Jorge, to protect FF 13 users I am going to remove Bootstrap support and
> switch to Restart version for the add-on till the bug is fixed. Is it OK?

Yeah, that's fine. Just mention that in the Notes to Reviewer so that we know why you're doing that.

Comment 23

[Alex Keybl [:akeybl]]

Comment on attachment 610350 [details] [diff] [review]
null check x2

[Triage Comment]
Low risk addition of a null check - approved for Aurora 13.

Comment 24

[Luke Wagner [:luke]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/974b5248b3ee

Comment 25

[Scoobidiver (away)]

(In reply to Luke Wagner [:luke] from comment #21)
> Oops, I should have added [leave open after merge].  I'll reopen so we can
> continue to track the issue and see if the patch fixes the topcrash.
In the trunk, there are no crashes after 14.0a1/20120329 on Windows and 14.0a1/20120326 on Mac/Linux. As the patch landed in 14.0a1/20120330, it's probably fixed.

Comment 26

[u279076]

qa+ for verification by checking crashstats.

Comment 27

[Virgil Dicu [:virgil] [QA]]

http://bit.ly/Kcn9aV

Fixed for 13 - only around 15 crashes cumulated for all 13 beta

But around *6000* crashes for 15a1 Nightly with the same signature in the last 2 weeks. Don't see this tracked in a separate bug by watching the Bugzilla tab in Socorro.

Would it make sense to log a new issue for that or keep investigating here?

Comment 28

[Bobby Holley (:bholley)]

(In reply to Virgil Dicu [:virgil] [QA] from comment #27)

> Would it make sense to log a new issue for that or keep investigating here?

If this is different than bug 752309, then we should definitely file a new bug for it. Almost certainly related to compartment-per-global.

Comment 29

[Virgil Dicu [:virgil] [QA]]

I couldn't reproduce bug 752309 (WFM for several people) so logged bug 757003 for the Firefox 15 crashes.

Marking this as verified in 13 (only 15 cumulated beta crashes)