Last Comment Bug 311172 - [FIX]Firefox crash when Flash disabled via userContent.css [@ nsObjectFrame::PluginNotAvailable ]
: [FIX]Firefox crash when Flash disabled via userContent.css [@ nsObjectFrame::...
: crash, fixed1.8.0.4, regression, testcase, verified1.8.1
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: 1.8 Branch
: x86 All
P1 critical (vote)
: mozilla1.8.1beta1
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Benjamin Smedberg [:bsmedberg]
Depends on:
  Show dependency treegraph
Reported: 2005-10-05 00:19 PDT by Elmar Ludwig
Modified: 2011-06-09 14:58 PDT (History)
11 users (show)
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (182 bytes, text/html)
2005-10-05 00:54 PDT, Elmar Ludwig
no flags Details
Patch for the issue I do see (1.50 KB, patch)
2006-04-14 22:19 PDT, Boris Zbarsky [:bz] (still a bit busy)
jst: review+
jst: superreview+
jst: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review

Description User image Elmar Ludwig 2005-10-05 00:19:02 PDT
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b5) Gecko/20051003 Firefox/1.4.1

Firefox crashes for me immediately after loading with
a current Firefox 1.5 branch build on Linux, but only when Flash is blocked via
the following code in chrome/userContent.css:

object[type="application/x-shockwave-flash"] {
    display: none !important;

When I remove this code from userContent.css, no crash occurs. Note that I am
not using the Flashblock extension (the only installed extension is ChatZilla)
unlike the very similar bug 282933. I filed a new report because this occurs
without Flashblock and my stack trace is different (though this may be related
to code changes from e.g. bug 1156). I also do not see a crash on the URL in
bug 282933, but the content may have changed since that bug was reported in
February 2005.

I've also tested some of the flash demos from this site:

None of them crash for me.

Talkback ID: TB10162428W

line 652]
line 2681]
line 3837]
line 3770]
line 3443]
line 119]
line 753]
line 921]
line 856]
line 385]
line 344]
line 689]
Comment 1 User image Elmar Ludwig 2005-10-05 00:54:03 PDT
Created attachment 198552 [details]

Minimal testcase that still crashes for me.
Comment 2 User image Ria Klaassen (not reading all bugmail) 2005-10-05 06:34:29 PDT
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051004
Firefox/1.4.1 ID:2005100419

I see the same in windows. It crashes only with plugin.default_plugin_disabled
to true.

Comment 3 User image Ria Klaassen (not reading all bugmail) 2005-10-05 07:01:59 PDT
Regression between 1.8b2_2005062912 and 1.8b2_2005062922.
Comment 5 User image Elmar Ludwig 2005-10-05 13:32:33 PDT
This leaves the patch for bug 277434 as the most likely candidate, I guess...

Looking at a build just before 20050629 it seems to me that the CSS rule from
userContent.css just doesn't match (since the <embed> tag is missing the type
attribute), so maybe the change in bug 277434 has just uncovered an older bug.

cc'ing jst as he might have an idea what is wrong here.
Comment 6 User image Mark Mentovai 2005-10-06 11:55:44 PDT
Not seeing this on the trunk.
Comment 7 User image Adam Guthrie 2005-12-08 12:09:17 PST
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051207 Firefox/1.6a1

No crash on either trunk or branch, but on branch the Flash still shows up.
Comment 8 User image Elmar Ludwig 2005-12-08 19:22:42 PST
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

The minimal testcase still crashes for me on the branch (Firefox 1.5 final).
It does not crash on the trunk, as Mark already mentioned im comment 6.
Comment 9 User image Adam Guthrie 2005-12-08 19:41:50 PST
Elmar, is it the same stack still? I still can't seem to get the userContent.css rules to prevent Flash from showing in branch...
Comment 10 User image Elmar Ludwig 2005-12-08 20:57:55 PST
Yes, exactly the same stack as before:

TB12761954H  (Firefox 1.5 final)
TB12762284K  (latest branch build 2005120804)

Hiding Flash via userContent.css works fine for me here on the branch (apart from the cases where it crashes the browser, of course).
Comment 11 User image Boris Zbarsky [:bz] (still a bit busy) 2006-04-14 22:16:30 PDT
So.... On the one hand I cannot reproduce this crash on branch (1.8.0 Firefox build).  Not only can I not reproduce it, but it's not obvious to me how that testcase could crash, given no extensions installed.

On the other hand, it's trivial to write a crash testcase here -- just have an event handler for the event we fire rearrange the DOM (which will kill the frame).  With any luck we will then crash when we set mIsBrokenPlugin or mState.  If we don't, we'll call mContent->GetDocument() which is virtual.  Not good.

The same issues exist on the aviary branch, of course; not sure how much we care about that.

Comment 12 User image Boris Zbarsky [:bz] (still a bit busy) 2006-04-14 22:19:43 PDT
Created attachment 218499 [details] [diff] [review]
Patch for the issue I do see

I haven't really tested this, largely because I have no idea how.  Do we have testcases for this plugin finder stuff somewhere?

Elmar, does this patch fix the crash for you?
Comment 13 User image Doron Rosenberg (IBM) 2006-04-15 08:42:14 PDT
We don't have textcases really - I usually just uninstall flash and visit a flash page and check that it installs correctly and all.
Comment 14 User image Boris Zbarsky [:bz] (still a bit busy) 2006-04-15 13:32:21 PDT
That's not going to cut it.  For me to have any confidence in landing this on a stable branch (which needs to happen), I need to test it against some testcases that exercise all major aspects of the plug-in finder stuff (at the least, I need to verify that there are no changes in look or behavior, on pages that use all three of object/embed/applet, on pages that do and do not set type attributes, and for both plug-ins that get a URI and plug-ins that do not).  This involves knowing what the plug-in finder is supposed to behave like, by the way (or rather which behaviors absolutely need to be tested).  Is that documented anywhere?

I can't believe we've been landing changes to this code on "stable" branches without having such tests, and I'd really appreciate if the original authors of the code would point to the tests they used to test their stuff before landing it; ideally said stuff would be in a relevant CVS test directory.  Let's do that while said original authors are still around, so we don't have to reverse-engineer this code a few years hence like we're having to do for so much code right now.
Comment 15 User image Doron Rosenberg (IBM) 2006-04-15 18:32:50 PDT
I have some testcases back at work, biesi perhaps has some for his XBL work (which might be the most affected by this change).  I guess toolkit/mozapps/plugins/tests would be a decent location.

There is no design document for the wizard UI, as I wrote the original stuff late during the aviary release cycle.  I could write something on the wiki and then QA could use it to build/write whatever they need.

Comment 16 User image timeless 2006-04-16 00:11:00 PDT
plugin tests should live in plugins or layout, since they are still valid whether or not we have some specific null plugin.
Comment 17 User image Boris Zbarsky [:bz] (still a bit busy) 2006-04-16 09:59:19 PDT
layout/html/tests would be a better location, imho, since we're testing layout code.

That is, the tests should live in layout; the descriptions of how the wizard behaves in response to those tests should live in mozapps as you said.  The relationship should be documented in a README in the tests dir.

> There is no design document for the wizard UI

Could one be written please?  Better late than never.

Note that imo this is a must for, so anything that could be done to expedite me being able to test this and land it so it doesn't miss would be much appreciated...
Comment 18 User image Johnny Stenback (:jst, 2006-04-17 14:05:26 PDT
Comment on attachment 218499 [details] [diff] [review]
Patch for the issue I do see

>+    // Make sure to fire the event AFTER we've finished touching out members.


Comment 19 User image Elmar Ludwig 2006-04-18 00:07:09 PDT
(In reply to comment #12)
> Elmar, does this patch fix the crash for you?

I'm fairly busy this week and building from source takes time. I will see what I can do though.
Comment 20 User image Christian :Biesinger (don't email me, ping me on IRC) 2006-04-19 09:20:16 PDT
the testcases I have are:

it's not much but the best that I have.
Comment 21 User image Doron Rosenberg (IBM) 2006-04-19 10:35:26 PDT is my current tests.

Four of them:
  - first tests a plugin that doesn't exist
  - second tests flash in embed
  - third tests flash in object
  - fourth tests flash/real in embed

I tried with the patch and see no behavior change. Note that it is best to open each test in a new tab, since we have a regression and aren't clearing the missing plugins list for each tab anymore on the 1.8 branch and trunk.
Comment 22 User image Boris Zbarsky [:bz] (still a bit busy) 2006-04-19 20:45:29 PDT
Awesome.  Thanks!  Checked in on the 1.8 branch.

Doron, is there a bug for the 1.8/trunk issue you mention in comment 21?
Comment 23 User image Doron Rosenberg (IBM) 2006-04-20 07:37:07 PDT
Bug 334674 is about the tabs not clearing their missing plugin data.
Comment 24 User image Daniel Veditz [:dveditz] 2006-04-20 11:50:23 PDT
Comment on attachment 218499 [details] [diff] [review]
Patch for the issue I do see

approved for 1.8.0 branch, a=dveditz for drivers
Comment 25 User image Boris Zbarsky [:bz] (still a bit busy) 2006-04-20 18:36:28 PDT
Fixed on 1.8.0 branch.
Comment 26 User image Tracy Walker [:tracy] 2006-08-22 10:02:04 PDT
verified with 2.0b2 builds from 20060821

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