Last Comment Bug 354595 - Crash setting flashEmbed[0] in Flash Player code called from nsJSObjWrapper::GetNewOrUsed
: Crash setting flashEmbed[0] in Flash Player code called from nsJSObjWrapper::...
Status: RESOLVED FIXED
: crash, testcase, verified1.8.1.8
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- critical (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 326633
  Show dependency treegraph
 
Reported: 2006-09-27 17:29 PDT by Jesse Ruderman
Modified: 2007-10-22 11:29 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (480 bytes, text/html)
2006-09-27 17:30 PDT, Jesse Ruderman
no flags Details
Possible fix. (2.75 KB, patch)
2007-04-17 16:05 PDT, Johnny Stenback (:jst, jst@mozilla.com)
jonas: review+
jonas: superreview+
dveditz: approval1.8.1.8+
Details | Diff | Review
Branch version, for the record (same change as above w/o some style cleanup). (2.02 KB, patch)
2007-08-07 15:07 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Review

Description Jesse Ruderman 2006-09-27 17:29:16 PDT
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
...
Comment 1 Jesse Ruderman 2006-09-27 17:30:06 PDT
Created attachment 240387 [details]
testcase
Comment 2 Raymond Walsh 2007-04-17 12:17:05 PDT
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.
 
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2007-04-17 16:05:11 PDT
Created attachment 261878 [details] [diff] [review]
Possible fix.
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2007-04-17 16:06:18 PDT
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.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2007-04-17 16:11:14 PDT
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 6 Daniel Veditz [:dveditz] 2007-08-07 14:55:12 PDT
Comment on attachment 261878 [details] [diff] [review]
Possible fix.

approved for 1.8.1.7, a=dveditz
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2007-08-07 14:55:18 PDT
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.
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2007-08-07 14:56:07 PDT
Marking bug fixed as there hasn't been any sign of this being a problem since this patch landed on the trunk.
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2007-08-07 15:07:57 PDT
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).
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2007-08-07 15:10:26 PDT
Fixed on branch.
Comment 11 Carsten Book [:Tomcat] 2007-09-13 07:35:29 PDT
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
Comment 12 Daniel Veditz [:dveditz] 2007-10-22 10:49:09 PDT
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
Comment 13 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2007-10-22 11:00:52 PDT
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.
Comment 14 Daniel Veditz [:dveditz] 2007-10-22 11:29:39 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.