545686 - [OOPP] crashed plugin UI doesn't work on some sites

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Maini Marco]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100211 Minefield/3.7a2pre (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100211 Minefield/3.7a2pre (.NET CLR 3.5.30729)

Crashed plugins UI doesn't appear when you visit some sites.

Reproducible: Always

Steps to Reproduce:
1. Visit http://www.megavideo.com/?v=NKCUTMML for example
2. Play video
3. From task manager, kill mozilla-runtime process.
Actual Results:  
A black screen is shown, instead of "crashed" plugin

Expected Results:  
Crashed plugin UI should be shown. This works correctly with Youtube video for example and also reload plugin options should work.

Comment 1

[Maini Marco]

Further tests: 
Dailymotion.com seems work correctly

Silverlight plugin has some issues:
this video http://www.nbcolympics.com/video/assetid=d0f80f63-3cf7-41b5-89b4-eb7316bb15fb.html#julia+mancuso+sings+karaoke after the "crash" shows a white plugin screen

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I can repro the black screen on http://www.megavideo.com/?v=NKCUTMML on linux x86-64.  I saw something similar happen on a silverlight page that had fallback content in silverlight's <object> frame, I wonder if something similar is going on here.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

On that page, after forcing a crash, I see these errors in the Error Console:

Error: overlay is null
Source File: chrome://browser/content/browser.js
Line: 8415
Error: overlay is null
Source File: chrome://browser/content/browser.js
Line: 8415

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oops, forgot --- this build was from http://hg.mozilla.org/mozilla-central/rev/93c7b23284b8.

Comment 5

[Tony Chung [:tchung]]

Forcing kill m-r, i still havent seen the crash plugin UI on any of these sites.

http://www.vimeo.com/8736190
http://www.dailymotion.com/video/xc4nb9_ngc-presents-imagining-pluto_shortfilms

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a2pre) Gecko/20100212 Minefield/3.7a2pre

Comment 7

[Justin Dolske [:Dolske]]

From my original-but-now-duped bug:

I've noticed on a few pages that if I manually kill the mozilla-runtime for
Flash, the new plugin-crashed UI is not shown. We're firing events and
browser.js is handling them, but I get a console error saying "JavaScript
error: chrome://browser/content/browser.js, line 8415: overlay is null".

That indicates the binding isn't attached, and indeed DOMi shows no binding. I
suspect what's happening is that the plugin disabled/blocked/crashed
pseudoclasses are all just copied from the missing-plugin version, which AIUI
has an exception in some cases to show the page's own fallback content instead
our plugin finder UI.

Comment 8

[Tony Chung [:tchung]]

http://www.kia.com/#/sorento/hop-in another example.  no crash UI

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

After a not-so-brief but enlightening foray into CSS and layout/frame code, the fix for this is delightfully simple.

I hadn't really looked at exactly what the :moz-has-handlerref pseudoclass meant, and I'm not sure earlier people patching this UI did either. 

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSRuleProcessor.cpp#1543 [mozHasHandlerRefMatches()]

...this code is responsible for controlling the pseudoclass, and all it does is check to see if there's an immediate <param name="pluginurl"> child of the <object>. But that hasn't really been relevant since we stopped using the Default Plugin.

So the net effect was that the rules controlling this UI suppressed the missing-plugin UI when such a <param> was not present. [Which, tbh, seems backward, since I think the intention was to suppress this UI when the Default Plugin knew how how install a plugin itself.]. And this also ended up suppressing the blocked/disabled/crashed UI in the same cases, which isn't at all what we want.

So we can just remove the pseudoclass, and I've filed bug 548133 to remove the bits of code that were special-casing <param name="pluginurl">.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #9)
> So the net effect was that the rules controlling this UI suppressed the
> missing-plugin UI when such a <param> was not present. [Which, tbh, seems
> backward, since I think the intention was to suppress this UI when the Default
> Plugin knew how how install a plugin itself.].

I thought the point was that <object>s that didn't have a <param name="pluginurl"> weren't necessarily being used for plugins... though I guess non-plugin <object>s can't be -moz-type-unsupported to begin with. I'm not sure whether that was the case when bug 309521 initially landed, though? This code is quite confusing, and I bet biesi/bz don't recall rationales from that far back...

Comment 11

[Boris Zbarsky [:bzbarsky]]

The correct behavior for an <object> pointing to unsupported stuff (and probably blocked/disabled stuff too, though those are not explicitly called out in specs), generally speaking, is to fall back.  We made an exception for <object>s with a pluginurl attribute for backwards-compat reasons (and because the plugin finder couldn't work with just notifications at the time, iirc, so needed the in-page UI more).

This patch makes us have that exceptional behavior for all <object>s in the "unsupported" state, though, right?  The only reason it doesn't fail Acid2 is that plug-ins with alternate content are not actually in the "unsupported" state.  But any <object> for an unknown type without alternate content will go from showing the notification and falling back to showing the binding.  Is that in fact the desired change?

For crashed plugin-ins, of course, all bets are off in terms of UI vs spec.  I suppose showing the fallback content on plug-in crash isn't really right, so we should probably just drop the :-moz-has-handlerref from object:-moz-handler-crashed.

Comment 12

[Justin Dolske [:Dolske]]

Bah, I've scope-slipped my own bug here.

Let me scale back to just the crashed-plugin UI (the original problem), and I'll file a new bug about the other cases.

Comment 13

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Comment 14

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/dd95723b7ce7

Comment 15

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/d7e553aff797

Comment 16

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

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 17

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430