Crash setting flashEmbed[0] in Flash Player code called from nsJSObjWrapper::GetNewOrUsed

RESOLVED FIXED

Status

()

Core
Plug-ins
--
critical
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Unassigned)

Tracking

(Blocks: 1 bug, {crash, testcase, verified1.8.1.8})

Trunk
PowerPC
Mac OS X
crash, testcase, verified1.8.1.8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

11 years ago
This crashes if "emb" is a Flash movie:

  var emb = document.getElementById("emb")
  emb[0] = 3;

Stack trace (note that the symbols for Flash Player are bogus):

0   ...romedia.Flash Player.plugin 	0x06b63b38 Flash_EnforceLocalSecurity + 374272
1   ...romedia.Flash Player.plugin 	0x06b638a4 Flash_EnforceLocalSecurity + 373612
2   ...romedia.Flash Player.plugin 	0x06cce754 native_ShockwaveFlash_TCallFrame + 452328
3   ...romedia.Flash Player.plugin 	0x06cce6f4 native_ShockwaveFlash_TCallFrame + 452232
4   org.mozilla.firefox            	0x007f6e50 nsJSObjWrapper::GetNewOrUsed(_NPP*, JSContext*, JSObject*) + 2956
5   libmozjs.dylib                 	0x2304c2e0 js_LookupPropertyWithFlags + 684
6   libmozjs.dylib                 	0x23030b2c js_CheckRedeclaration + 84
7   libmozjs.dylib                 	0x23048ea8 js_HasOwnPropertyHelper + 1156
8   libmozjs.dylib                 	0x2302ff28 js_Invoke + 1828
9   libmozjs.dylib                 	0x2303a4d0 js_Interpret + 37512
...
(Reporter)

Updated

11 years ago
Summary: Crash setting flashEmbed[0] → Crash setting flashEmbed[0] in Flash Player code called from nsJSObjWrapper::GetNewOrUsed
(Reporter)

Comment 1

11 years ago
Created attachment 240387 [details]
testcase

Comment 2

11 years ago
It appears possible for PL_DHashTableOperate to return null (when the table is on the verge of overflowing and the object exists), but this condition is not trapped.



dhcp145:~/cvsrepos/mozilla/modules/plugin/base/src apple$ cvs diff -u8p ./nsJSNPRuntime.cpp 
Index: nsJSNPRuntime.cpp
===================================================================
RCS file: /cvsroot/mozilla/modules/plugin/base/src/nsJSNPRuntime.cpp,v
retrieving revision 1.23
diff -u -8 -p -r1.23 nsJSNPRuntime.cpp
--- nsJSNPRuntime.cpp   27 Mar 2007 15:33:41 -0000      1.23
+++ nsJSNPRuntime.cpp   17 Apr 2007 18:39:22 -0000
@@ -948,16 +948,19 @@ nsJSObjWrapper::GetNewOrUsed(NPP npp, JS
   }
 
   nsJSObjWrapperKey key(obj, npp);
 
   JSObjWrapperHashEntry *entry =
     NS_STATIC_CAST(JSObjWrapperHashEntry *,
                    PL_DHashTableOperate(&sJSObjWrappers, &key, PL_DHASH_ADD));
 
+  if (!entry) {
+      return nsnull;
+  }
   if (PL_DHASH_ENTRY_IS_BUSY(entry) && entry->mJSObjWrapper) {
     // Found a live nsJSObjWrapper, return it.
 
     return _retainobject(entry->mJSObjWrapper);
   }
 
   // No existing nsJSObjWrapper, create one.
 
Created attachment 261878 [details] [diff] [review]
Possible fix.
Attachment #261878 - Flags: superreview?(jonas)
Attachment #261878 - Flags: review?(jonas)
And that fix is of course based on Raymonds findings. I'm not sure yet whether it actually fixes this bug, but it's the right thing to do either way.
Attachment #261878 - Flags: superreview?(jonas)
Attachment #261878 - Flags: superreview+
Attachment #261878 - Flags: review?(jonas)
Attachment #261878 - Flags: review+
Fix checked in on trunk. Leaving bug open as I don't know if this actually fixes this bug. If it appears to, feel free to mark this bug fixed.
Comment on attachment 261878 [details] [diff] [review]
Possible fix.

approved for 1.8.1.7, a=dveditz
Attachment #261878 - Flags: approval1.8.1.7+
Comment on attachment 261878 [details] [diff] [review]
Possible fix.

Requesting approval to land this on branches, I've gotten off-line reports of people seeing this crash on the branch, but not on the trunk since this landed there.
Marking bug fixed as there hasn't been any sign of this being a problem since this patch landed on the trunk.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Created attachment 275675 [details] [diff] [review]
Branch version, for the record (same change as above w/o some style cleanup).

This is the same as the trunk version with the exception of the first hunk that only changes code formatting (and doesn't apply to the branch).
Fixed on branch.
Keywords: fixed1.8.1.7

Updated

10 years ago
Attachment #275675 - Attachment is patch: true
Attachment #275675 - Attachment mime type: application/octet-stream → text/plain
verified fixed 1.8.1.7 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.7pre) Gecko/20070913 BonEcho/2.0.0.7pre ID:2007091303 

no crash on testcase - adding verified keyword
Keywords: fixed1.8.1.7 → verified1.8.1.7
I don't know if this is the cause, but Mac Flash crashes have skyrocketed in 2.0.0.8 compared to 2.0.0.7.  Even though total crashes in a 10 day window favor the old version 45K to 27K, crashes in Flash on a Mac have gone from 223 to 844.

FF 2.0.0.7             FF 2.0.0.8
223 Flash crashes      844 Flash crashes
13.5% of Mac crashes   60% of Mac crashes
0.5% of all crashes    3% of all crashes
Yeah, I noticed quite a few Mac people complaining about crashing after the update in the Mozilla feedback newsgroups.
Carsten mentioned that it could be related to bug 400554.
I don't have a Mac, so I can't really help with testing.
I filed bug 400714 to cover comment 12 -- this fix may or may not have been the cause so it should not be tracked here.
You need to log in before you can comment on or make changes to this bug.