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

RESOLVED FIXED in Firefox 13

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Scoobidiver (away), Unassigned)

Tracking

(4 keywords)

13 Branch
mozilla14
crash, regression, reproducible, topcrash
Points:
---

Firefox Tracking Flags

(firefox13+ verified, firefox14 verified)

Details

(Whiteboard: [startupcrash][qa+], crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 2

5 years ago
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.
tracking-firefox13: --- → ?
Keywords: reproducible, topcrash

Updated

5 years ago
tracking-firefox13: ? → +

Comment 3

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

Comment 4

5 years ago
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.)
(Reporter)

Comment 5

5 years ago
(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]

Comment 6

5 years ago
(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.
(Reporter)

Comment 7

5 years ago
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

5 years ago
Ah, that makes more sense.
(Reporter)

Updated

5 years ago
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?

Comment 11

5 years ago
(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

5 years ago
Created attachment 610329 [details] [diff] [review]
null check
Attachment #610329 - Flags: review?(bobbyholley+bmo)
Attachment #610329 - Flags: review?(bobbyholley+bmo) → review+

Comment 13

5 years ago
Created attachment 610350 [details] [diff] [review]
null check x2

Actually two are needed.
Attachment #610350 - Flags: review?(bobbyholley+bmo)
Attachment #610350 - Flags: review?(bobbyholley+bmo) → review+

Comment 14

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed31daf07bd
Target Milestone: --- → mozilla14

Updated

5 years ago
Attachment #610329 - Attachment is obsolete: true

Comment 15

5 years ago
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?

Comment 16

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

Comment 17

5 years ago
- 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

5 years ago
Created attachment 610529 [details]
Youtube Video Replay with Addons Manager

Comment 19

5 years ago
Created attachment 610530 [details]
Youtube Video Replay No Addons Manager
https://hg.mozilla.org/mozilla-central/rev/7ed31daf07bd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 21

5 years ago
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+

Comment 24

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/974b5248b3ee
(Reporter)

Comment 25

5 years ago
(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
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
status-firefox13: --- → fixed
status-firefox14: --- → verified
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)
status-firefox13: fixed → verified
You need to log in before you can comment on or make changes to this bug.