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

Status: VERIFIED FIXED

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

Description

[philippe (part-time)]

Attachment: 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

Comment 1

[philippe (part-time)]

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.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Or smaug's bug 462517, since we crash in nsObjectLoadingContent.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Stacktrace?

I guess, I must learn how to compile Camino and install FlashBlock.

Comment 5

[Chris Lawson (gone)]

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!

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a patch

I need to still understand why weakframe is "alive", when frame is null.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: even more strict

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for trunk

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Ok, the patch isn't right and the bug is somewhere else.
New patch coming soon...

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Or, maybe it is right, after all.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

This case might happen also in FF without any extensions if
document is moved to bfcache before between |new nsAsyncInstantiateEvent| and |nsAsyncInstantiateEvent::Run| calls.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 371641 [details] [diff] [review]
a patch

This should be enough, both for trunk and branches.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for trunk with assertion

Comment 17

[Samuel Sidler (old account; do not CC)]

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...

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for branches with assertion

Comment 19

[Samuel Sidler (old account; do not CC)]

This should block 1.9.1 since it's a regression from bug 462517.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/7ed443926742

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 371667 [details] [diff] [review]
for branches with assertion

Asking approval - null check to fix the crash.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 23

[Chris Lawson (gone)]

(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).

Comment 24

[Samuel Sidler (old account; do not CC)]

Oh, right. This blocks 1.9.0.9 and we're respinning to pick it up.

Comment 25

[philippe (part-time)]

(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.

Comment 26

[Al Billings [:abillings - ex-MoCo]]

Thanks for the help, Philippe.

Comment 27

[Samuel Sidler (old account; do not CC)]

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.

Comment 28

[Nick Thomas [:nthomas] (UTC+12)]

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

Comment 29

[Alexander Sack]

code doesn't exist on 1.8 branches

Comment 30

[philippe (part-time)]

Verified fixed on Gecko 1.9.0.10 with Camino Version 2.0b3pre (1.9.0.10pre 2009040900), using the STR in comment 0

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 371667 [details] [diff] [review]
for branches with assertion

(blockers don't need approvals)

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

This should be fixed also on 1.9.1. The patch for Bug 489988 contains the fix.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.