[Webkit-unassigned] [Bug 16815] Crash with navigator.plugins and navigator.mimeTypes after plugins.refresh

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=16815


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #19188|review?                     |review-
               Flag|                            |




------- Comment #15 from darin at apple.com  2008-02-24 18:38 PDT -------
(From update of attachment 19188)
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.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.