Closed Bug 487345 Opened 12 years ago Closed 12 years ago

Crash [@ nsObjectLoadingContent::Instantiate] when enabling Flashblock after visiting this URL

Categories

(Core :: Plug-ins, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: phiw2, Assigned: smaug)

References

(Blocks 1 open bug, )

Details

(5 keywords, Whiteboard: [sg:dos])

Crash Data

Attachments

(4 files, 2 obsolete files)

Attached file crash log
This seems specific to this particular page. At least, I haven't been able to repro with other sites.
I don't see anything weird in it though: stock Dreamweaver code/js.

Reported in the forums
http://forums.mozillazine.org/viewtopic.php?f=12&t=1185155

STR
1. make sure Flashblock is not enabled
2. visit http://www.repetitive-strain.com/
3. load another page
4. Open preferences, enable Flashblock

AR: Camino crashes (it may take a few seconds)
ER: let's just go on with life.

Version 2.0b3pre (1.9.0.8pre 2009031700) no crash
Version 2.0b3pre (1.9.0.8pre 2009031800)

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2009-03-17+00%3A00%3A00&maxdate=2009-03-18+00%3A00%3A00&cvsroot=%2Fcvsroot

Possibly bug 416942
I tried to reproduce this with Minefield latest and Fx 3.0.10pre (2009040704 GranParadiso/3.0.10pre) with FlashBlock 1.5.9, but could not reproduce this.

There is difference with Camino: in Camino, enabling FlashBlock is 'live', whereas in Firefox, one has to reload a page before FlashBlock is applied to a page.
Or smaug's bug 462517, since we crash in nsObjectLoadingContent.
Severity: major → critical
Keywords: regression
Summary: Crash when enabling Flashblock after visiting this URL → Crash [@ nsObjectLoadingContent::Instantiate] when enabling Flashblock after visiting this URL
I backed out the -r1.95 -r1.96 nsObjectLoadingContent.cpp diff locally and no longer crash, so it does appear to be bug 462517 that triggered this.

(I can't set 462517 in the "blocks" field here because it's a security bug and apparently setting that relationship requires "access" to the security bug.)
Blocks: 462517
Stacktrace?

I guess, I must learn how to compile Camino and install FlashBlock.
The best stacktrace we have (unfortunately, I don't think it's out of a debug build) is attachment 371579 [details].

Thanks for looking into this so fast!
Attached patch a patchSplinter Review
I need to still understand why weakframe is "alive", when frame is null.
Attached patch even more strict (obsolete) — Splinter Review
Assignee: nobody → Olli.Pettay
Attachment #371641 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Asking blocking1.9.0.9? so that it is evaluated if bug 462517
should be postponed to 1.9.0.10 or if the null check fix here should be
taken to the branch.
Flags: blocking1.9.0.9?
Attached patch for trunk (obsolete) — Splinter Review
Ok, the patch isn't right and the bug is somewhere else.
New patch coming soon...
Group: core-security
Or, maybe it is right, after all.
Comment on attachment 371649 [details] [diff] [review]
for trunk

Ok, so the case is that the relevant document is in bfcache, so doc->GetPrimaryShell returns null, and that is why ::GetExistingFrame returns null.
bz, what you think about this?

Note, IMO, FlashBlock shouldn't touch documents which are in bfcache.
Attachment #371649 - Flags: superreview?(bzbarsky)
Attachment #371649 - Flags: review?(bzbarsky)
This case might happen also in FF without any extensions if
document is moved to bfcache before between |new nsAsyncInstantiateEvent| and |nsAsyncInstantiateEvent::Run| calls.
Attachment #371649 - Flags: superreview?(bzbarsky)
Attachment #371649 - Flags: review?(bzbarsky)
Attachment #371641 - Attachment is obsolete: false
Attachment #371641 - Flags: superreview?(bzbarsky)
Attachment #371641 - Flags: review?(bzbarsky)
Comment on attachment 371641 [details] [diff] [review]
a patch

This should be enough, both for trunk and branches.
Attachment #371649 - Attachment is obsolete: true
Attachment #371646 - Attachment is obsolete: true
Group: core-security
Attachment #371641 - Flags: superreview?(bzbarsky)
Attachment #371641 - Flags: superreview+
Attachment #371641 - Flags: review?(bzbarsky)
Attachment #371641 - Flags: review+
Comment on attachment 371641 [details] [diff] [review]
a patch

This makes sense, but can we do the following:

1)  Assert that either !frame or !mFrame.IsAlive() or frame == mFrame?

2)  File a followup bug to consider revoking instantiate events when going into bfcache?
Blocks: 487411
Dan: Need to decide if we need this for 1.9.0.9 or if we want to back out bug 462517 there.

Sending this to core...
Component: Annoyance Blocking → Plug-ins
Product: Camino → Core
QA Contact: annoyance.blocking → plugins
This should block 1.9.1 since it's a regression from bug 462517.
Flags: blocking1.9.1?
http://hg.mozilla.org/mozilla-central/rev/7ed443926742
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #371667 - Flags: approval1.9.1?
Comment on attachment 371667 [details] [diff] [review]
for branches with assertion

Asking approval - null check to fix the crash.
Attachment #371667 - Flags: approval1.9.0.9?
Whiteboard: [sg:dos]
This a verification by code review bug since we don't have a way to make this happen? 

Philippe, can you verify that this fixes the issue for Camino?
(In reply to comment #22)
> This a verification by code review bug since we don't have a way to make this
> happen?

It's fairly easily reproducible in Camino, so it should be verifiable with Camino (not just by code review).
Oh, right. This blocks 1.9.0.9 and we're respinning to pick it up.
Flags: blocking1.9.0.9?
Flags: blocking1.9.0.9+
Flags: blocking1.9.0.10+
(In reply to comment #22)

> Philippe, can you verify that this fixes the issue for Camino?

The patch has not landed yet for Camino (Camino is on the Gecko 1.9.0 Branch). 
Building Camino myself with the patch and testing the STR in comment 0:  the issue is definitely fixed, no more crashes.
Thanks for the help, Philippe.
Comment on attachment 371667 [details] [diff] [review]
for branches with assertion

Approved for 1.9.0.9 and 1.9.0.10. a=ss for release-drivers.

This needs to get landed on 1.9.0 and on the relbranch for 1.9.0.9.
Attachment #371667 - Flags: approval1.9.0.9?
Attachment #371667 - Flags: approval1.9.0.9+
Attachment #371667 - Flags: approval1.9.0.10+
Comment on attachment 371667 [details] [diff] [review]
for branches with assertion

Landed on CVS HEAD:
new revision: 1.97; previous revision: 1.96

Landed on GECKO190_20090406_RELBRANCH for 3.0.9 build2:
new revision: 1.96.2.1; previous revision: 1.96
code doesn't exist on 1.8 branches
Flags: wanted1.8.1.x-
Flags: wanted1.8.0.x-
Attachment #371641 - Flags: approval1.9.1?
Attachment #371641 - Flags: approval1.9.1?
Verified fixed on Gecko 1.9.0.10 with Camino Version 2.0b3pre (1.9.0.10pre 2009040900), using the STR in comment 0
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
Comment on attachment 371667 [details] [diff] [review]
for branches with assertion

(blockers don't need approvals)
Attachment #371667 - Flags: approval1.9.1?
This should be fixed also on 1.9.1. The patch for Bug 489988 contains the fix.
Keywords: fixed1.9.1
Verified fixed on trunk and 1.9.1:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090520 Minefield/3.6a1pre ID:20090520033508

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090520 Shiretoko/3.5b5pre ID:20090520035357

Crash stats also report no more errors.
Status: RESOLVED → VERIFIED
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: mozilla1.9.1 → mozilla1.9.2a1
Crash Signature: [@ nsObjectLoadingContent::Instantiate]
You need to log in before you can comment on or make changes to this bug.