Bug 56575 – REGRESSION: "Hidden" NPAPI plugin not loaded (conflicts with HTML5 hidden="" attribute)

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Plug-ins

[Pere Martir]

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

[Pere Martir]

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.

[Philippe Normand]

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

[Pere Martir]

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>

[Pere Martir]

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

[Eric Seidel (no email)]

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.

[Pere Martir]

Created attachment 88988 [details]
Patch before plugin loading is moved from renderer to DOM

The proposed patch implements what I mentioned in comment #3.

[WebKit Review Bot]

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.

[Pere Martir]

Created attachment 88992 [details]
Tabs removed

[Adam Barth]

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?

[Pere Martir]

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.

[Pere Martir]

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?

[Eric Seidel (no email)]

Do we know what revision changed this behavior?

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Anders knows all things plugins and may know why this changed and when (or if this is the right behavior at all?)

[Eric Seidel (no email)]

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.

[Pere Martir]

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

[Eric Seidel (no email)]

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?

[Sébastien Moutte]

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.

[Eric Seidel (no email)]

I assume that your extension does not use the Chrome extension auto-update system?
http://code.google.com/chrome/extensions/autoupdate.html

[Eric Seidel (no email)]

Actually, I thought if your extension was published through the Chrome Web Store you got auto-updating built in?

[Sébastien Moutte]

Unfortunately our extension doesn't implement the extension auto-update system :(

[Sébastien Moutte]

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 ?

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Sébastien Moutte]

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

[Eric Seidel (no email)]

(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

[Eric Seidel (no email)]

I've filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=15133 asking for spec clarification.

[Ian 'Hixie' Hickson]

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.

[Ian 'Hixie' Hickson]

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?

[Ahmad Saleem]

NPAPI support is removed from Safari 14 onward and it is not supported in Webkit Builds like WebkitGTK as well. I think this can be marked as "RESOLVED WONTFIX". Thanks!