Bug 29486 – [Qt] When an object tag refers to an invalid plugin then it can cause the webpage to not be shown correctly

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

[Tor Arne Vestbø]

This bug report originated from issue QTBUG-2609
http://bugreports.qt.nokia.com/browse/QTBUG-2609

--- Description ---

When an object tag refers to an invalid plugin then it can cause the webpage to not be shown correctly.

[Jocelyn Turcotte]

Created attachment 46596 [details]
Compilable test case (from original bug)

[Jocelyn Turcotte]

I can reproduce the problem in 4.6 on Windows.

Raising the severity to major as I don't see how the plugin in the test case is invalid.
The same problem is reported there:
http://lists.trolltech.com/pipermail/qt-interest/2009-May/006681.html

[Luiz Agostini]

confirmed in r80003.

[Luiz Agostini]

Created attachment 84281 [details]
patch

[Luiz Agostini]

Created attachment 84412 [details]
before

[Luiz Agostini]

Created attachment 84413 [details]
after

[Luiz Agostini]

The problem with the attached example is that the <object> tag is not closed. The HTML parser will not consider <object /> as a closed tag as an XML parser would do. As the result everything that comes after <object> is considered its children and no renderer is created for it.

Regarding the proposed patch, the rationale for it is that if FrameLoaderClientQt::createPlugin() returns false then RenderEmbeddedObject::setShowsMissingPluginIndicator() will get called and the result will be that RenderEmbeddedObject::pluginCrashedOrWasMissing() will return true. It seems to make sense to RenderEmbeddedObject::pluginCrashedOrWasMissing() to return true if the plugin is missing. :).

However, it may be our choice to return the pluginView even if it did not succeeded on loading. If we do so then QtWebKit will produce an output similar to what you can see in the attachment named "before". It was chrome option to return 0 as I proposed in the patch and the Safari option was to show the text "Missing Plug-in".

We may choose to let the patch get in or not but I think that we can close this bug anyway.

[Antonio Gomes]

Comment on attachment 84281 [details]
patch

Luiz, it should be easily testable, no?

[Luiz Agostini]

(In reply to comment #8)
> (From update of attachment 84281 [details])
> Luiz, it should be easily testable, no?

Yes, for sure. But I think that we do not actually have a bug. We may decide to make it this or that way but, as it is today in the trunk, it is not wrong.

We probably should just close this bug.

[Antonio Gomes]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 84281 [details] [details])
> > Luiz, it should be easily testable, no?
> 
> Yes, for sure. But I think that we do not actually have a bug. We may decide to make it this or that way but, as it is today in the trunk, it is not wrong.
> 
> We probably should just close this bug.

Or you can add a test to ensure the behavior to be adopted is preserved.

[Luiz Agostini]

After some more testing I consider that we should just close this bug as invalid.

The problem here is just that the <object> tag is not closed. <object /> would be considered a closed tag by an XML parser but not by an HTML parser. Just replacing <object /> by <object></object> solves the problem and the page is shown as expected.

I consider that the way it is now implemented is good and that the proposed patch (that I sent) will not improve QtWebKit behavior. I will remove the review flag.

Tor Arne, Jocelyn, do you agree? May I close this bug?

[Luiz Agostini]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (From update of attachment 84281 [details] [details] [details])
> > > Luiz, it should be easily testable, no?
> > 
> > Yes, for sure. But I think that we do not actually have a bug. We may decide to make it this or that way but, as it is today in the trunk, it is not wrong.
> > 
> > We probably should just close this bug.
> 
> Or you can add a test to ensure the behavior to be adopted is preserved.

At the and I think that the patch does not help that much.

Thanks tonikitoo. :)