Bug 16815 – Crash with navigator.plugins and navigator.mimeTypes after plugins.refresh

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

[Simon Hausmann]

The implementation of navigator.plugins and navigator.mimeTypes in kjs_navigator.cpp uses pointers to the Mime and Plugin objects retrieved from the PluginInfoStore. navigator.plugins.refresh() deletes those objects but there may still be Plugin and MimeType objects around with stale pointers. The attached testcase reproduces the crash.

We ran into this while trying to make plugins work better for the Qt platform and the second attachment to this report includes a proposed patch. The patch only demonstrates the concept, it is incomplete with regards to the implementation of PluginStoreQt and the implementations on the other platforms. If the concept finds approval we will try to implement the proposed API changes for the other platforms.

We propose to change PluginInfoStore to return values and store those values in the JavaScript binding objects. In addition we're adding a Frame pointer parameter that allows binding the PluginInfoStore to the context of a Frame or in the case of the Qt port to an entire QWebPage instance.

[Simon Hausmann]

Created attachment 18363 [details]
Testcase to reproduce navigator.plugins crash

[Simon Hausmann]

Created attachment 18364 [details]
Proposed patch (concept)

[Darin Adler]

Comment on attachment 18364 [details]
Proposed patch (concept)

Here are a few comments:

If you're going to put frame pointers into all of these separately reference counted objects, you either need to make the frame pointers get zeroed out when the frame goes away or you need to make them RefPtr<Frame>.

The direction I'd like to see this move is to make these objects more like normal DOM objects, with automatically generated bindings.

I don't see why the list of plug-ins should be per-frame or per-page. Having each web page keep a separate list of plug-ins seems unnecessary. I'm concerned that we're taking too many lower-level things and involving the page in them.

On the other hand, I could be wrong. If we are going to make the plug-in data structures be per-page we should do it cross platform with as much cross-platform code as possible.

If the plug-in information is really per-page, then the functions should be members of Page or a class with a page pointer in them. I don't think it's a good pattern to have a class PlugInInfoStore and pass a frame pointer to all the functions of that class.

This looks like a Qt-only patch.

+        PluginBase(ExecState *exec, Frame* frame);

Normally we would not name these arguments when their purpose is completely clear from their type.

[Simon Hausmann]

The main reason for making plugins and other pieces depend on the page instead of them being just global data is modularity. This is not important if WebKit is only used in a browser application. But for example in an application that shows controlled content in one view with additional Qt based plugins that might access e.g. the filesystem and at the same time show a help documentation window with HTML documentation loaded from the web, the use of plugins has to be restricted to the application view. It must not be possible for JavaScript executed within the documentation window to see which plugins are loaded in the other window, which is the case right now due to the global mime/plugins list in the JavaScript binding.

> This looks like a Qt-only patch.

Most of the patch is about fixing a WebKit security issue/crash. The addition of the frame pointer is something one might argue about, but being able to specify plugins on a per-page basis is a requirement for the Qt port. We also think that this might be an important feature for any WebKit based framework.

We'll try to replace the raw frame pointer with a reference counter pointer in the next patch revision.

[Simon Hausmann]

Created attachment 18436 [details]
new (concept) patch

This new patch includes the following changes:

1) Auto-generate bindings for window.navigator and its sub-objects (plugins/mimeTypes)
2) Replace PlugInInfoStore with PluginData
3) Remove unused page/Plugin.h
4) Move all plugin code into WebCore/plugins
5) Move navigator object into WebCore/page

This is still just a proof of concept and requires implementation of PluginData::platformInit() and PluginData::platformRefresh() on all platforms.

[David Kilzer (:ddkilzer)]

<rdar://problem/5686501>

[David Kilzer (:ddkilzer)]

Crash log loading Attachment #18363 [details] on Mac OS X 10.4.11 (8S165) with Safari 3.0.4 (523.12.2) with a local debug build of WebKit r29451:

Thread 0 Crashed:
0   com.apple.WebCore         	0x01722fac WebCore::StringImpl::characters() + 20 (StringImpl.h:75)
1   com.apple.WebCore         	0x0152e768 WebCore::String::operator KJS::UString() const + 96 (String.cpp:558)
2   com.apple.WebCore         	0x015b5f58 KJS::Plugin::getValueProperty(KJS::ExecState*, int) const + 204 (kjs_navigator.cpp:434)
3   com.apple.WebCore         	0x019f9f70 KJS::JSValue* KJS::staticValueGetter<KJS::Plugin>(KJS::ExecState*, KJS::JSObject*, KJS::Identifier const&, KJS::PropertySlot const&) + 92 (lookup.h:151)
4   com.apple.JavaScriptCore  	0x006250a0 KJS::PropertySlot::getValue(KJS::ExecState*, KJS::JSObject*, KJS::Identifier const&) const + 180 (property_slot.h:49)
5   com.apple.JavaScriptCore  	0x00576dc0 KJS::JSObject::get(KJS::ExecState*, KJS::Identifier const&) const + 84 (object.cpp:164)
6   com.apple.JavaScriptCore  	0x00676d80 KJS::DotAccessorNode::inlineEvaluate(KJS::ExecState*) + 184 (nodes.cpp:814)
7   com.apple.JavaScriptCore  	0x005b5e30 KJS::DotAccessorNode::evaluate(KJS::ExecState*) + 40 (nodes.cpp:819)
8   com.apple.JavaScriptCore  	0x005b5d04 KJS::ArgumentListNode::evaluateList(KJS::ExecState*, KJS::List&) + 100 (nodes.cpp:864)
9   com.apple.JavaScriptCore  	0x00676cb4 KJS::ArgumentsNode::evaluateList(KJS::ExecState*, KJS::List&) + 124 (nodes.h:515)
10  com.apple.JavaScriptCore  	0x00677988 KJS::FunctionCallDotNode::inlineEvaluate(KJS::ExecState*) + 632 (nodes.cpp:1216)
11  com.apple.JavaScriptCore  	0x005c2e8c KJS::FunctionCallDotNode::evaluate(KJS::ExecState*) + 40 (nodes.cpp:1228)
12  com.apple.JavaScriptCore  	0x005b0dd0 KJS::ExprStatementNode::execute(KJS::ExecState*) + 84 (nodes.cpp:3640)
13  com.apple.JavaScriptCore  	0x00588b08 KJS::statementListExecute(WTF::Vector<WTF::RefPtr<KJS::StatementNode>, (unsigned long)0>&, KJS::ExecState*) + 128 (nodes.cpp:3593)
14  com.apple.JavaScriptCore  	0x00588c20 KJS::BlockNode::execute(KJS::ExecState*) + 48 (nodes.cpp:3618)
15  com.apple.JavaScriptCore  	0x005b03cc KJS::ForNode::execute(KJS::ExecState*) + 332 (nodes.cpp:3807)
16  com.apple.JavaScriptCore  	0x00588b08 KJS::statementListExecute(WTF::Vector<WTF::RefPtr<KJS::StatementNode>, (unsigned long)0>&, KJS::ExecState*) + 128 (nodes.cpp:3593)
17  com.apple.JavaScriptCore  	0x00588c20 KJS::BlockNode::execute(KJS::ExecState*) + 48 (nodes.cpp:3618)
18  com.apple.JavaScriptCore  	0x005adabc KJS::ProgramNode::execute(KJS::ExecState*) + 56 (nodes.cpp:4507)
19  com.apple.JavaScriptCore  	0x005d25b4 KJS::Interpreter::evaluate(KJS::ExecState*, KJS::UString const&, int, KJS::UChar const*, int, KJS::JSValue*) + 860 (interpreter.cpp:123)
20  com.apple.WebCore         	0x015b89b4 WebCore::KJSProxy::evaluate(WebCore::String const&, int, WebCore::String const&) + 292 (kjs_proxy.cpp:90)
21  com.apple.WebCore         	0x011b5578 WebCore::FrameLoader::executeScript(WebCore::String const&, int, WebCore::String const&) + 128 (FrameLoader.cpp:758)
22  com.apple.WebCore         	0x012367b0 WebCore::HTMLTokenizer::scriptExecution(WebCore::DeprecatedString const&, WebCore::HTMLTokenizer::State, WebCore::DeprecatedString, int) + 388 (HTMLTokenizer.cpp:520)
23  com.apple.WebCore         	0x01238354 WebCore::HTMLTokenizer::scriptHandler(WebCore::HTMLTokenizer::State) + 1664 (HTMLTokenizer.cpp:470)
24  com.apple.WebCore         	0x012389b4 WebCore::HTMLTokenizer::parseSpecial(WebCore::SegmentedString&, WebCore::HTMLTokenizer::State) + 1208 (HTMLTokenizer.cpp:319)
25  com.apple.WebCore         	0x0123afc8 WebCore::HTMLTokenizer::parseTag(WebCore::SegmentedString&, WebCore::HTMLTokenizer::State) + 7960 (HTMLTokenizer.cpp:1248)
26  com.apple.WebCore         	0x0123b92c WebCore::HTMLTokenizer::write(WebCore::SegmentedString const&, bool) + 1504 (HTMLTokenizer.cpp:1464)
27  com.apple.WebCore         	0x011a7e10 WebCore::FrameLoader::write(char const*, int, bool) + 1288 (FrameLoader.cpp:996)
28  com.apple.WebCore         	0x011b4660 WebCore::FrameLoader::endIfNotLoadingMainResource() + 128 (FrameLoader.cpp:1033)
29  com.apple.WebCore         	0x011b4748 WebCore::FrameLoader::end() + 44 (FrameLoader.cpp:1018)
30  com.apple.WebCore         	0x01149f10 WebCore::DocumentLoader::finishedLoading() + 92 (DocumentLoader.cpp:322)
31  com.apple.WebCore         	0x011ab230 WebCore::FrameLoader::finishedLoading() + 96 (FrameLoader.cpp:2791)
32  com.apple.WebCore         	0x0133ba14 WebCore::MainResourceLoader::didFinishLoading() + 244 (MainResourceLoader.cpp:291)
33  com.apple.WebCore         	0x01459eec WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*) + 60 (ResourceLoader.cpp:363)
34  com.apple.WebCore         	0x01457990 -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] + 204 (ResourceHandleMac.mm:456)
35  com.apple.Foundation      	0x92c1a7ec -[NSURLConnection(NSURLConnectionInternal) _sendDidFinishLoadingCallback] + 188
36  com.apple.Foundation      	0x92c18a58 -[NSURLConnection(NSURLConnectionInternal) _sendCallbacks] + 556
37  com.apple.Foundation      	0x92c187b0 _sendCallbacks + 156
38  com.apple.CoreFoundation  	0x907df30c __CFRunLoopDoSources0 + 384
39  com.apple.CoreFoundation  	0x907de83c __CFRunLoopRun + 452
40  com.apple.CoreFoundation  	0x907de2bc CFRunLoopRunSpecific + 268
41  com.apple.HIToolbox       	0x932a0b20 RunCurrentEventLoopInMode + 264
42  com.apple.HIToolbox       	0x932a01b4 ReceiveNextEventCommon + 380
43  com.apple.HIToolbox       	0x932a0020 BlockUntilNextEventMatchingListInMode + 96
44  com.apple.AppKit          	0x937a6bc4 _DPSNextEvent + 384
45  com.apple.AppKit          	0x937a6888 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 116
46  com.apple.Safari          	0x000095e0 0x1000 + 34272
47  com.apple.AppKit          	0x937a2dcc -[NSApplication run] + 472
48  com.apple.AppKit          	0x93893974 NSApplicationMain + 452
49  com.apple.Safari          	0x0009bad4 0x1000 + 633556
50  com.apple.Safari          	0x000022fc 0x1000 + 4860

[Sam Weinig]

I love the direction this patch is going!  One comment though, the class Shared has been replaced with RefCounted for quite a while, you should update accordingly.

[Sam Weinig]

I will do a more thorough review later on today.

[Darin Adler]

Comment on attachment 18436 [details]
new (concept) patch

Basic approach looks great.

Here are a few comments.

For new names, please use "PlugIn" rather than "Plugin". Because it's plug-in, not plugin. I know we're inconsistent about this, but it would be good to be consistent in the new code. Unless the real DOM goes the other way, in which case maybe we should rename it in the other direction.

I'm not a big fan of the name PlugInData, because of the vagueness of the word "data". Maybe PlugInSpecifications? What type of data is it? I'm especially annoyed that we have both PlugInData and PlugInInfo, and they are two different things!

Formatting has a lot of strange things in it.

+        inline Navigator* clientInformation() const
+        { return navigator(); }
 
Functions in class declarations are automatically inline, so no need to say "inline" here. We normally put stuff like this all on one line, or format it as a multiple line function declaration. This style, where the body is all on one line, but a line after the declaration, is not the usual.

+ *  This file is part of the KDE libraries

Please don't include that in new source files.

When moving a large block of code to a new file, please use a "svn copy" so we don't lose all the history of the code. Also it makes it easier to review because I can see the changes.

It seems a little strange to have almost all the functions of Navigator be const members, and almost all the data members be mutable. How about just skipping "const" for this object, since it's a DOM object and the DOM has no real notion of const.

+        ~Navigator();
+        ~MimeType();

We normally repeat the "virtual" when overriding things like the virtual destructor.

+        /*readonly attribute MimeTypeArray mimeTypes; */

We normally don't check in commented-out code, even in IDL files, if it can be avoided.

+    const Vector<MimeClassInfo*> &mimes = m_pluginData->mimes();

Misplaced & character here. Should go next to the type.

+Plugin *MimeType::enabledPlugin() const
+        Plugin *enabledPlugin() const;

Misplaced * character here.

+        MimeType(const RefPtr<PluginData>& pluginData, int index);

If you're passing ownership, the parameter type should be PassRefPtr<PlugInData>, not const RefPtr&. And the argument name should be omitted in cases like this one.

+#include "RefPtr.h"

It's <wtf/RefPtr.h>.

+#include "Shared.h"

It hasn't been "Shared.h" for a while. It's <wtf/RefCounted.h>.

Headers with functions that return a String don't need to include PlatformString.h. They can just forward-declare String.

+    class PluginData : public Shared<PluginData>
+    {

Brace goes on the same line as the class name.

+    for (int i = 0; i < m_mimes.size(); ++i)

Mixing int with size_t is going to create a warning. Better to either put the size into an int local variable or use size_t consistently.

For the various places where we are looking for MIME types, etc. the code should probably use more modern data structures like hashes rather than iterating over lists doing == on each element. Maybe the MIME types should be AtomicString to save memory if the same type appears over and over again?

It seems really poor for each frame to have its own entire copy of the plug-in database. Is there any way to allow sharing for the common cases where all frames are identical?

When you refresh, I don't think it is correct to leave all other frames with old data about plug-ins. It's good that you're fixing this so frames can be different from each other, but moving from too much sharing to no sharing at all doesn't seem good.

+    // ### hack

First, we use "FIXME" for such things. Second, we try to be more specific about what's up. Why is this code in the DOM? Why not elsewhere?

[Simon Hausmann]

Created attachment 18527 [details]
patch including mac support

[Simon Hausmann]

We have continued to work on the patch, including code style fixes and an implementation for the Mac port that also removes some now unused code.

We decided to stick to PluginData for now as we think that in the future we should simply merge this with PluginDataBase, which appears to be the correct name and place. But that of course requires bigger changes outside the scope of this bug report / crash.

We thought about the use of a hash for the plugins and the mime type, but given the linear nature of the JavaScript API here it appears safer to stay with the plain lists to preserve order and behaviour.

The data itself is now shared across Page instances, which simplified the sharing itself and also implements the reloading of frames that contain plugins in a platform independent way.

[Maciej Stachowiak]

Although "plug-in" is the naming convention on Mac OS X, "plugin" is much more commonly used, especially in computing contexts and most particularly in the browser context. Enough that Google thinks "plug-in" is a misspelling for "plugin":

http://www.google.com/search?&q=plugin
http://www.google.com/search?&q=plug-in

I don't think we need to make the code bow to the quirky Mac UI convention on this.

[Simon Hausmann]

Created attachment 19188 [details]
updated patch that compiles against trunk

This patch is just an update to the previous patch that adds the crash as a testcase to LayoutTests and fixes the compilation after the latest round of changes in trunk.

The build on the mac is not included in this patch (working on that) and the windows implementation of PluginData is also still missing (it should use the newly landed PluginPackage of course).

[Darin Adler]

Comment on attachment 19188 [details]
updated patch that compiles against trunk

This patch is great. Sorry I didn't review it earlier.

The patch will need to be revised to preserve the new Windows-only quirk in appVersion, which will require a custom binding.

+        MimeType(PluginData* pluginData, unsigned index);

This should be a PassRefPtr<PluginData>, not a raw pointer.

+        Plugin* enabledPlugin() const;

Since this produces a new reference counted object, the return type should be a PassRefPtr.

+        m_navigator = new Navigator(m_frame);

New refcounted classes should use the "create" idiom, where there's a function to create a new object that returns a PassRefPtr and the constructor should be private.

+        readonly attribute Navigator navigator;
+        readonly attribute Navigator clientInformation;

Is it correct that these are not [Replaceable]? Does this change behavior from the old version or preserve it?

+    const String userAgent = m_frame->loader()->userAgent(m_frame->document() ? m_frame->document()->url() : KURL());

There's no good reason to use "const String" here. We could declare many of our local variables const, but we normally don't. But instead of String, you could use const String&. I don't think it would be more efficient, but it's often a good idiom. It's probably even better to just use "this->userAgent()" to initialize userAgent; if you did that you could even skip the m_frame null check.

+    class Frame;
+    class String;
+    class PluginData;
+    class PluginArray;
+    class MimeTypeArray;

We normally use alphabetical order for these things.

+    HashSet<Page*>::iterator end = allPages->end();
+    for (HashSet<Page*>::iterator it = allPages->begin(); it != end; ++it) {

Can calling reload() trigger anything that changes the contents of allPages? Like maybe open a new window? If it can, then this code needs to be written so it can cope with that. The behavior of iterators in a hash table that changes is undefined -- could crash. A safe idiom is to copy the set into a vector and iterate the vector.

+        for (Frame* frame = (*it)->mainFrame(); frame; frame = frame->tree()->traverseNext()) {
+            if (reload && frame->loader()->containsPlugins())

I suggest putting the "reload" if statement outside the loop.

If reload() can change the state of frames then I think you need to use a RefPtr<Frame> to avoid a possible null-dereference. We might also need to copy the frames into a vector to avoid problems in case the frame tree changes.

+    : RefCounted<MimeType>(0)

Please use a count of 1 for new classes you're adding in the future. this is OK on this patch, but should be revised for future patches.

Sorry, I have to go now and I only got down to PluginData.h -- still got about a third of the patch to review.

review- because of the above issues.

[Darin Adler]

Comment on attachment 19188 [details]
updated patch that compiles against trunk

One thing I'm not happy about is all the classes that have the name XXXInfo and XXXData. I find them confusing. Like -- what's the difference between PluginData and PluginInfo? Anything we can do to rationalize these would be a step in the right direction.

+        PluginData(const Page* page);

We'd normally omit the parameter name for something like this.

+        inline const Vector<PluginInfo*>& plugins() const
+        { return m_plugins; }
+        inline const Vector<MimeClassInfo*>& mimes() const
+        { return m_mimes; }

No reason for the inline keyword on these lines; it has no effect. Also, normally the function body would be to the right rather than on a separate line below.

Those are all my comments. I finished reviewing now.

I'd love to help you finish the patch; if I have time soon I might take it and fix some of the issues I mentioned and do some testing on Mac OS X.

[Simon Hausmann]

Created attachment 19345 [details]
diff against the previous patch

This attachment is just a diff against the previous posted patch, to make it easier to see the changes that happened.

[Simon Hausmann]

Created attachment 19346 [details]
updated patch

The complete patch including Darin's comments, a Gtk+ build fix and a crash fix in DOMImplementation::createDocument where the frame pointer can be null.

[Simon Hausmann]

I have included your suggestions in the latest patch. Thanks for this round of review!

The readonly attribute change was indeed a bug. A little test that overwrites window.navigator indicates that it's indeed replaceable like the other window properties.

The const of the string in window.navigator.appVersion came actually from the existing code in kjs_navigator.cpp, but I've removed the const now. I'm not sure about caching the return value of FrameLoaderClient::userAgent due to the dependency of the URL though. But perhaps I misunderstood your suggestion.

I have also changed Page::refreshPlugins() to continue to operate on the allPages hashset, but instead of doing any modification it now just collects all the frames that need a reload in a separate vector. I think that should protected against potential page/frame creations/deletions as you suggested.

I agree about the confusing namng between PluginData and PluginInfo, but I think PluginData in the future should simply be merged with PluginDatabase to clean up the code and resolve this naming problem.

The latest patch still needs an implementation of PluginData on Windows. That should be pretty straightforward from the existing PluginInfoStore.cpp code. The mac implementation should require only changes to the xcode project files, the existing mac/PluginData.mm implementation should work fine.

[Darin Adler]

Comment on attachment 19346 [details]
updated patch

Changes look great.

+    // Version is everything in the user agent string past the "Mozilla/" prefix.
+    String userAgent = frame->loader()->userAgent(frame->document() ? frame->document()->url() : KURL());
+    return jsString(userAgent.substring(userAgent.find('/') + 1));

I'd suggest leaving this part of appVersion in the Navigator object, and calling jsString(imp->appVersion()) here in the JSNavigator binding.

+            for (Frame* frame = (*it)->mainFrame(); frame; frame = frame->tree()->traverseNext())
+                if (frame->loader()->containsPlugins())
+                    framesNeedingReload.append(frame);

Our formatting rules would say that this for statement needs braces (the if statement doesn't).

+#include <wtf/PassRefPtr.h>

Compiling things that include parameters and return values of type PassRefPtr only requires <wtf/Forward.h>, although that's not critical.

+        MimeType(PassRefPtr<PluginData> pluginData, unsigned index);

We'd normally omit the parameter name "pluginData" here.

+    for (Vector<RefPtr<Frame> >::iterator it = framesNeedingReload.begin(); it != framesNeedingReload.end(); ++it)
+        (*it)->loader()->reload();

There's no real benefit to using an iterator to iterate a vector. It's probably better practice to use indexing instead for vectors.

Obviously I can't say review+ for a patch that doesn't yet contain the changes needed to compile on Mac OS X and Windows. So I'm not sure what to do next here. I guess I can apply the patch myself and try to finish it on each platform.

[Darin Adler]

(In reply to comment #19)
> The const of the string in window.navigator.appVersion came actually from the
> existing code in kjs_navigator.cpp, but I've removed the const now. I'm not
> sure about caching the return value of FrameLoaderClient::userAgent due to the
> dependency of the URL though. But perhaps I misunderstood your suggestion.

I wasn't suggesting you cache the result. My point was that for functions that return String you can do this:

    const String& result = function();
    otherFunction(result);

Using "const String&" rather than "String".

Some on the Safari team here at Apple have suggested using this idiom as the default for things like String, AtomicString. In cases where the function actually returns a reference rather than a String, it avoids a bit of refcount churn, at the expense of being ever so slightly more vulnerable to object lifetime problems if the object that returned the string is modified before the reference is used.

[Simon Hausmann]

Created attachment 19374 [details]
updated diff against previous patch

This is a diff against the last patch containing updates for the Gtk+ build with Automake as well as the suggested coding style changes.

[Simon Hausmann]

Created attachment 19375 [details]
new patch including Gtk+ Automake build fix and more coding style fixes

Thanks for the suggestions, I've added them to this latest patch.

I wanted to change the inclusions of PassRefPtr.h to Forward.h, but realized it doesn't work because the inline create() function that replaces the constructor uses adoptRef, which calls actually functions on PassRefPtr.

[Simon Hausmann]

Created attachment 19400 [details]
new patch including Mac X build fixes

This patch includes only a small fix to the testcase and changes to the xcode project files to fix the build on Mac OS X. Only Windows is missing now for patch completion.

[Simon Hausmann]

Created attachment 19433 [details]
patch including support for Windows

I have implemented PluginDataWin.cpp and modified the visual studio project files. Due to machine problems I couldn't really _test_ the resulting DLL though, but it compiled and linked for me.

[Mark Rowe (bdash)]

Comment on attachment 19433 [details]
patch including support for Windows

There are no ChangeLog entries.

[Darin Adler]

Comment on attachment 19433 [details]
patch including support for Windows

Great patch! Seems almost ready to go.

+    const String &userAgent = m_frame->loader()->userAgent(m_frame->document() ? m_frame->document()->url() : KURL());

Should be const String&, with the space after the &.

+  return m_frame->settings()->isJavaEnabled();

Should be indented four spaces.

+    for (unsigned int i = 0; i < framesNeedingReload.size(); ++i)

We normally just use "unsigned", but also since this is a Vector, should be size_t.

+        m_pluginData = new PluginData(this);

Should be using PluginData::create instead of calling new directly. I don't think this will even compile.

+MimeType::MimeType(PassRefPtr<PluginData> pluginData, unsigned index)
+    : RefCounted<MimeType>(0)

This is going to be a problem. The constructor here starts with a count of 0, but the create function does an adoptRef. Need to remove the initialization of the refcount to 0. Same problem with PluginArray and perhaps other classes. Please double check this.

+    for (unsigned int i = 0; i < plugins.size(); ++i)
+        if (plugins[i] == mimes[m_index]->plugin)
+            return Plugin::create(m_pluginData.get(), i);

Same comment about unsigned int and size_t as above. Also, we use braces around multi-line bodies, even if they are only a single statement, so the for should have braces.

Could evaluate mimes[m_index]->plugin outside the loop.

+        String type() const;
+        String suffixes() const;
+        String description() const;

Could return const String& from these for better efficiency since these are stored in a persistent data structure.

+#include "PluginData.h"

No need to include the entire PluginData header just to compile a RefPtr to one.

+        unsigned long length() const;

unsigned long in IDL is the same as unsigned in C++, not unsigned long. So this should just be unsigned, not unsigned long.

+    // FIXME: Generated JSPlugin.cpp doesn't include JSMimeType.h for toJS
+    KJS::JSValue* toJS(KJS::ExecState*, MimeType*);

What's the deal with this? Have you discussed it with Sam Weinig?

It'd be more efficient to use a constructor for MimeClassInfo rather than constructing one with null strings and then assigning to it.

The class MimeClassInfo uses the mixed class Mime as in Qt rather than the all capitals MIME used elsewhere in WebKit.

review- because of the thing that won't compile (new with a private constructor) and the refcount problems (0 with adoptRef).

[Simon Hausmann]

Created attachment 19583 [details]
difference to the previous patch

This attachment is just a difference to the previous page _excluding_ the changes that were necessary to adapt the entire patch to the latest renames of files.

[Simon Hausmann]

Created attachment 19584 [details]
updated patch rebased against trunk and including the latest suggestions

[Simon Hausmann]

Created attachment 19586 [details]
diff against previous patch including idl changes that were missing

[Simon Hausmann]

Created attachment 19587 [details]
updated patch rebased against trunk including the latest suggestion and idl fixes

Another update for the complete patch. I forgot the changes to the length property in the IDL files. This is included now. (actually the changes in the implementation to use unsigned instead of unsigned long)

[Simon Hausmann]

Some comments on the latest changes that go beyond the smaller fixes:

1) The reference counting was indeed incorrect and WebCore::PluginData was accidentially also not using the create()/adoptRef() pattern. This is fixed now.

2) The forward declaration of the toJS function we included because JSPlugin.cpp's indexGetter() function calls item in WebCore::Plugin. That function returns a MimeType pointer which needs to be converted to a JSValue using the toJS function generated in JSMimeType.h. So JSPlugin.cpp needs to include JSMimeType.h due to the indexGetter. We could not find a way to add this inclusion so a plain forward declaration seemed like the easiest fix.

3) I agree about the use of a constructor for MimeClassInfo, but then I'm not sure how important the performance is here since this is normally called only once on startup. On the other hand I find code that initializes the members by name more readable than passing just arguments to the constructor.

[Darin Adler]

Comment on attachment 19587 [details]
updated patch rebased against trunk including the latest suggestion and idl fixes

I'm annoyed by the use of Mime in this new code and MIME in all existing WebCore code. I would have preferred to stick with MIME.

 1598         String pluginName;
 1599         if (m_frame->page())
 1600             pluginName = m_frame->page()->pluginData()->pluginNameForMimeType(mimeType);
15991601         if (!pluginName.isEmpty() && !pluginName.contains("QuickTime", false)) 
16001602             return true;

This could have been done with a nested if; obviously the "return true" is not needed if the page is 0.

In fact, we could have just added the "page" check to the if above this.

 67     for (unsigned i = 0; i < mimes.size(); ++i)
 68         if (mimes[i] == mime)
 69             return MimeType::create(m_pluginData.get(), i).get();

These should have braces.

r=me

Is there a way to make a regression test for this? We normally require them.

[Simon Hausmann]

Landed in r30923.

I have included the coding style fixes suggested.

This bug as well as the patch include a LayoutTest.