Bug 56575 - REGRESSION: "Hidden" NPAPI plugin not loaded (conflicts with HTML5 hidden="" attribute)
Summary: REGRESSION: "Hidden" NPAPI plugin not loaded (conflicts with HTML5 hidden="" ...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 44351 45049
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-17 11:03 PDT by Pere Martir
Modified: 2012-10-01 01:54 PDT (History)
15 users (show)

See Also:


Attachments
Patch before plugin loading is moved from renderer to DOM (5.24 KB, patch)
2011-04-11 04:16 PDT, Pere Martir
no flags Details | Formatted Diff | Diff
Tabs removed (5.24 KB, patch)
2011-04-11 05:05 PDT, Pere Martir
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pere Martir 2011-03-17 11:03:56 PDT
Chrome guys suggested me to report this issue here. The original issue of Chromium: http://code.google.com/p/chromium/issues/detail?id=76351

A "hidden" NPAPI plugin is loaded by the code below:

  <object type="application/x-my-extension" id="pluginId" hidden="true">

It worked in Chrome 9, but not in Chrome 10. However, if we remove the attribute "hidden", it works in Chrome 10. Is it a bug or by design ?

Tested under Chrome 10
* LOADED     - <embed  type="application/x-my-extension" id="pluginId">
* LOADED     - <embed  type="application/x-my-extension" id="pluginId" hidden="true">
* LOADED     - <embed  type="application/x-my-extension" id="pluginId" hidden="false">
* LOADED     - <object type="application/x-my-extension" id="pluginId">
* NOT LOADED - <object type="application/x-my-extension" id="pluginId" hidden="true">
* NOT LOADED - <object type="application/x-my-extension" id="pluginId" hidden="false">
Comment 1 Pere Martir 2011-03-22 06:33:48 PDT
I'd like to know that if it's likely that this will be changed in the future ? The comments in the original bug report suggested that the plugin should be loaded, even it's "hidden".

We use <object hidden="true"> to load our windowless plugins and it seems that the existence of the attribute "hidden" (no matter it's true of false) now prevents the plugin from being loaded in Chrome 10.

I will appreciate if somebody could nicely point me to the related source code file of this problem. I am very new to WebKit source.
Comment 2 Philippe Normand 2011-03-27 09:41:20 PDT
Maybe something's wrong in the parsing of the element attributes?

Have a look in Source/WebCore/html/HTMLObjectElement.cpp and its parent classes.

I'm not very familiar with the NPPlugin code in WebKit though ;)
Comment 3 Pere Martir 2011-03-28 08:23:31 PDT
It seems that the changeset 73459 (bug 40511: Implement HTML5 hidden attribute) accidentally affects the OBJECT element. The EMBED element has not been affected because HTMLEmbedElement.cpp handles the "hidden" attribute by setting CSS height and width to zero, while HTMLObjectElement.cpp delegates the parsing to HTMLElement.cpp, which in changeset 73459 maps "hidden" attribute to CSS "display:none" and causes the plugin not loaded, known in bug 27775.

The fix has already been planned in bug 45049, but a quick fix will be implementing the same hack of HTMLEmbedElement.cpp in HTMLObjectElement.cpp.

Committed r73459: <http://trac.webkit.org/changeset/73459>
Comment 4 Pere Martir 2011-04-04 09:21:11 PDT
(In reply to comment #3)
> The fix has already been planned in bug 45049, but a quick fix will be implementing the same hack of HTMLEmbedElement.cpp in HTMLObjectElement.cpp.

I am working on this "fix" - mapping hidden attribute to CSS width=height=0 instead of mapping to CSS display:none, is this patch likely to be accepted before bug 45049 is implemented ?
Comment 5 Eric Seidel (no email) 2011-04-04 09:53:51 PDT
I'm not sure that's a good fix.  But maybe it's the right thing to do for the meanwhile.  I expect it will be a while before we finish fixing plugin handling.  It's not top of my todo list.
Comment 6 Pere Martir 2011-04-11 04:16:22 PDT
Created attachment 88988 [details]
Patch before plugin loading is moved from renderer to DOM

The proposed patch implements what I mentioned in comment #3.
Comment 7 WebKit Review Bot 2011-04-11 04:18:30 PDT
Attachment 88988 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plug..." exit_code: 1

Source/WebCore/html/HTMLObjectElement.cpp:89:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Pere Martir 2011-04-11 05:05:26 PDT
Created attachment 88992 [details]
Tabs removed
Comment 9 Adam Barth 2011-04-11 10:59:47 PDT
Comment on attachment 88992 [details]
Tabs removed

View in context: https://bugs.webkit.org/attachment.cgi?id=88992&action=review

> LayoutTests/plugins/hidden-plugin-loaded.html:9
> +   function printResults()

Is this racy?  Does the load event wait for plugins?
Comment 10 Pere Martir 2011-04-12 02:29:24 PDT
Comment on attachment 88992 [details]
Tabs removed

View in context: https://bugs.webkit.org/attachment.cgi?id=88992&action=review

>> LayoutTests/plugins/hidden-plugin-loaded.html:9
>> +   function printResults()
> 
> Is this racy?  Does the load event wait for plugins?

I copied it from LayoutTests/plugins/embed-prefers-plugins-for-images.html . In fact many tests under LayoutTests/plugins assume it (the load event waits for plugins) without explicit waits.
Comment 11 Pere Martir 2011-04-18 03:37:16 PDT
Is this unit test valid ? What else I should do to improve this patch ? I'd like to know  have this patch committed to the trunk before the bug 45049 is fixed. How is the possibility please?
Comment 12 Eric Seidel (no email) 2011-05-04 01:17:00 PDT
Do we know what revision changed this behavior?
Comment 13 Eric Seidel (no email) 2011-05-04 01:18:34 PDT
Comment on attachment 88992 [details]
Tabs removed

View in context: https://bugs.webkit.org/attachment.cgi?id=88992&action=review

> Source/WebCore/html/HTMLObjectElement.cpp:139
> +            // FIXME: Not dynamic, since we add this but don't remove it, but it may be OK for now
> +            // that this rarely-used attribute won't work properly if you remove it.
> +            addCSSLength(attr, CSSPropertyWidth, "0");
> +            addCSSLength(attr, CSSPropertyHeight, "0");

I'm really not a big fan of this hack.

It's not clear to me why this is needed.  When did behavior change?  Do we know why it changed?  It should be relatively trivial to run Tools/Scripts/bisect-builds or git bisect and find the revision at which the bahavior changed in WebKIt.  Then we would better understand what change may be necessary to restore old behavior (or if the behavior change was intentional).
Comment 14 Eric Seidel (no email) 2011-05-04 01:19:14 PDT
Anders knows all things plugins and may know why this changed and when (or if this is the right behavior at all?)
Comment 15 Eric Seidel (no email) 2011-05-04 02:20:58 PDT
http://trac.webkit.org/changeset/73459 seems to be the related change.

I take it that hidden=true just didn't used to do anything?  In which case, this isn't really a regression.
Comment 16 Pere Martir 2011-05-04 03:18:37 PDT
Comment on attachment 88992 [details]
Tabs removed

View in context: https://bugs.webkit.org/attachment.cgi?id=88992&action=review

>> Source/WebCore/html/HTMLObjectElement.cpp:139
>> +            addCSSLength(attr, CSSPropertyHeight, "0");
> 
> I'm really not a big fan of this hack.
> 
> It's not clear to me why this is needed.  When did behavior change?  Do we know why it changed?  It should be relatively trivial to run Tools/Scripts/bisect-builds or git bisect and find the revision at which the bahavior changed in WebKIt.  Then we would better understand what change may be necessary to restore old behavior (or if the behavior change was intentional).

I've seen that you've found that it was due to changeset 73459, which accidentally broke the behavior of how "hidden" attribute works with OBJECT element. EMBED element has not been affect because HTMLEmbedElement.cpp has this "hack". Since HTML5 says the plugin should be instantiated no matter that it's visible or not, maybe this hack is the correct thing to do now. 

Related comment of this hack: https://bugs.webkit.org/show_bug.cgi?id=40511#c7

Quotation: "...The selected plugin is instantiated even if the element is hidden with a 'display:none' CSS style."
Comment 17 Eric Seidel (no email) 2011-05-05 11:26:57 PDT
My understanding is that this bug was brought to our attention by a company shipping a product affected by this "regression".

The proper fix is to fix display: none iframes to still load plugins.  But that's likely a lot of work.

The simplest solution would be for the company to update their users.  But I'm told they can't do that.

Sometimes for large websites or applications we add temporary quirks to webkit.  I'm not saying that's the right thing to do here, but it's something to consider.

What would the url or extension name that we'd be quirking on here?  Who's actually affected by this bug?
Comment 18 Sébastien Moutte 2011-05-06 04:15:25 PDT
The application affected by this "regression" is our OfferBox Browser, http://www.offerbox.com/en/about-offerbox/
It implements browser extensions for Chrome, Internet Explorer and Firefox in order to suggest relevant offers to our users.
The name of our Chrome extension is "Offerbox" with ID:bjeikeheijdjdfjbmknpefojickbkmom

We have a part of our product users base for which our product extension stopped working when Chrome upgraded from version 9 to 10. We have no way to update automatically these users; some of them have already upgraded to a newer version of our product which is not affected by this hidden attribute but some users will probably never download and install the new version so they will just stop using our product or they would decide to uninstall it as it stopped working.

We understand that the proper fix for this "regression" represents a lot of work and we would really appreciate if you could integrate this quick fix. Our patch handles the attribute "hidden" in the exact same way than the one which is already implemented for the EMBED element.

Thank you for your support.
Comment 19 Eric Seidel (no email) 2011-05-06 09:19:45 PDT
I assume that your extension does not use the Chrome extension auto-update system?
http://code.google.com/chrome/extensions/autoupdate.html
Comment 20 Eric Seidel (no email) 2011-05-06 09:21:21 PDT
Actually, I thought if your extension was published through the Chrome Web Store you got auto-updating built in?
Comment 21 Sébastien Moutte 2011-05-06 11:24:31 PDT
Unfortunately our extension doesn't implement the extension auto-update system :(
Comment 22 Sébastien Moutte 2011-05-10 13:58:56 PDT
Could anyone review our patch or advise us about another way to fix this regression properly ?

As the fix proposed is indentical to the implementation done for EMBED element, I though it would be welcome. The patch passed the unit tests.

Did we miss something here ?
Comment 23 Eric Seidel (no email) 2011-06-01 16:36:27 PDT
It is possible for Chrome to add a quirk for specific extensions, like how WebKit on mac often has to quirk for specific applications on Mac OS X.  I have no idea if Chrome folks are interested in quirking for extensions.   But that might be another avenue to explore.

Looking at:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#the-embed-element

and

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#the-object-element

It seems that HTML5 says nothing about a hidden attribute for either <object> or <embed>.  It's possible our behavior should just be removed.
Comment 24 Eric Seidel (no email) 2011-06-01 16:38:47 PDT
Looks like hidden is a global attribute:
http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#the-hidden-attribute

So our current behavior is correct.
Comment 25 Sébastien Moutte 2011-06-02 10:13:21 PDT
(In reply to comment #23)

I can read from http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#the-object-element that object element shouldn't be affected by hidden attribute.
"Note:The above algorithm is independent of CSS properties (including 'display', 'overflow', and 'visibility'). For example, it runs even if the element is hidden with a 'display:none' CSS style, and does not run again if the element's visibility changes."

Same for EMDED element http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#the-embed-element

"Note:The embed element is unaffected by the CSS 'display' property. The selected plugin is instantiated even if the element is hidden with a 'display:none' CSS style."

Did I miss something ?
Comment 26 Eric Seidel (no email) 2011-12-09 15:17:09 PST
(In reply to comment #25)
> (In reply to comment #23)
> 
> I can read from http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#the-object-element that object element shouldn't be affected by hidden attribute.
> "Note:The above algorithm is independent of CSS properties (including 'display', 'overflow', and 'visibility'). For example, it runs even if the element is hidden with a 'display:none' CSS style, and does not run again if the element's visibility changes."
> 
> Same for EMDED element http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#the-embed-element
> 
> "Note:The embed element is unaffected by the CSS 'display' property. The selected plugin is instantiated even if the element is hidden with a 'display:none' CSS style."
> 
> Did I miss something ?

Corrrect.  The spec currently disagrees with reality.  Ian is working on a fix:
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-November/033732.html
Comment 27 Eric Seidel (no email) 2011-12-09 15:23:38 PST
I've filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=15133 asking for spec clarification.
Comment 28 Ian 'Hixie' Hickson 2012-01-31 14:54:12 PST
So... is it just this site, or is it multiple sites? Can we break this one site?

The only other solution I see is:

   object[hidden] { display: inline; width: 0; height: 0; overflow: hidden; }

...in the ua.css file.
Comment 29 Ian 'Hixie' Hickson 2012-08-08 17:39:07 PDT
What's the status here? Did we decide it was a WONTFIX, did it somehow become WORKSFORME, or is it a known issue that's causing us pain?