Closed Bug 733793 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect)

13 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla14
Tracking Status
firefox13 + verified
firefox14 --- verified

People

(Reporter: scoobidiver, Unassigned)

Details

(4 keywords, Whiteboard: [startupcrash][qa+])

Crash Data

Attachments

(3 files, 1 obsolete file)

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
(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.
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) rsDownloader@163.com (RS+MU Downloader 2.4.1)
     60% (12/20) vs.  26% (12/47) jsdeobfuscator@adblockplus.org (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) tilt@mozilla.com (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.
It looks like the crash rate has a distinct downward trend.  Could a fix have been landed?
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.)
(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.
Whiteboard: [startupcrash]
(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.
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.
Ah, that makes more sense.
Summary: Crash in js::CrossCompartmentWrapper::getPropertyDescriptor @ xpc::WrapperFactory::PrepareForWrapping → Crash in js::CrossCompartmentWrapper::getPropertyDescriptor @ xpc::WrapperFactory::PrepareForWrapping (mainly correlated to Youtube Video Replay)
Adding Youtube Video Replay developer to CC list.
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?
(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.
Attached patch null check (obsolete) — Splinter Review
Attachment #610329 - Flags: review?(bobbyholley+bmo)
Attachment #610329 - Flags: review?(bobbyholley+bmo) → review+
Attached patch null check x2Splinter Review
Actually two are needed.
Attachment #610350 - Flags: review?(bobbyholley+bmo)
Attachment #610350 - Flags: review?(bobbyholley+bmo) → review+
Attachment #610329 - Attachment is obsolete: true
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
Attachment #610350 - Flags: approval-mozilla-aurora?
- David, I am not good at Firefox Internals but to me it somehow relates to AddonsManager and mozIJSSubScriptLoader.loadSubScript
- 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?
https://hg.mozilla.org/mozilla-central/rev/7ed31daf07bd
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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 on attachment 610350 [details] [diff] [review]
null check x2

[Triage Comment]
Low risk addition of a null check - approved for Aurora 13.
Attachment #610350 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
qa+ for verification by checking crashstats.
Whiteboard: [startupcrash] → [startupcrash][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?
(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.
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)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: