Component: General → Plug-ins
Product: Firefox → Core
QA Contact: general → plugins
Version: unspecified → 1.9.0 Branch
Good catch! This seems like it's pretty easy to patch.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Created attachment 329679 [details] [diff] [review] Easiest fix We could avoid the duplicate lookup by trying to keep track of whether the JS_NewObject call caused any wrappers to get collected, but I don't think it's worth it.
Comment on attachment 329679 [details] [diff] [review] Easiest fix It'd be trivial to sample sNPObjWrappers.generation before and after the JS_NewObject() and only do the re-lookup if the table changed. r+sr=jst either way.
Created attachment 329803 [details] [diff] [review] With jst's suggestion
I tried this latest patch with the Firefox 18.104.22.168 codebase, and it appears to resolve the problem, no more crash. I put a breakpoint in that entry reload code, and it hit a few times, at roughly the same frequency Firefox would crash before, so I'm confident this is indeed fixing it.
This is a trivial and safe obvious crash fix that we should include in dot release etc.
Flagging as a potential security problem to make it more likely downstreams don't pass this one up.
Version: 1.9.0 Branch → unspecified
Anyway to write an automated test for this one?
Flags: blocking22.214.171.124? → blocking126.96.36.199+
Unfortunately not at this point.
Pushed as http://hg.mozilla.org/index.cgi/mozilla-central/rev/20bff6157770 Thanks for the great analysis, Antoine!
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Comment on attachment 329803 [details] [diff] [review] With jst's suggestion Blake, does this patch apply to both 1.8 and 1.9?
Comment on attachment 329803 [details] [diff] [review] With jst's suggestion This applies directly to the 1.9 branch.
Attachment #329803 - Flags: approval188.8.131.52?
1.9 drivers: It isn't possible to write an automated test for this bug because it depends heavily on GC timing (and we don't have a testsuite for plugin bugs yet).
Comment on attachment 329803 [details] [diff] [review] With jst's suggestion Approved for 184.108.40.206. Please land in CVS. a=ss (And can we get a 1.8 branch patch for this?)
Attachment #329803 - Flags: approval220.127.116.11? → approval18.104.22.168+
Fix checked into the 1.9 branch.
Comment on attachment 329803 [details] [diff] [review] With jst's suggestion This applies as-is to the 1.8 branch.
Attachment #329803 - Flags: approval22.214.171.124?
Comment on attachment 329803 [details] [diff] [review] With jst's suggestion Approved for 126.96.36.199, a=dveditz for release-drivers.
Attachment #329803 - Flags: approval188.8.131.52? → approval184.108.40.206+
Fix checked into the 1.8 branch.
Comment on attachment 329803 [details] [diff] [review] With jst's suggestion a=asac for 220.127.116.11
Attachment #329803 - Flags: approval18.104.22.168+
This is difficult to verify for 22.214.171.124. Any suggestions?
(In reply to comment #20) > This is difficult to verify for 126.96.36.199. Any suggestions? This is difficult to verify anywhere. Unless someone wants to do comment 5 style verification, I don't think it's worth it.
You need to log in before you can comment on or make changes to this bug.