Bug 80428 - HTMLPluginElement is not destroyed on reload or navigation if getNPObject is called
Summary: HTMLPluginElement is not destroyed on reload or navigation if getNPObject is ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Michael
URL:
Keywords:
Depends on: 82020 82035
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-06 10:30 PST by Dave Michael
Modified: 2012-03-26 01:15 PDT (History)
13 users (show)

See Also:


Attachments
Patch (2.38 KB, patch)
2012-03-06 11:04 PST, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (2.68 KB, patch)
2012-03-15 08:57 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (9.03 KB, patch)
2012-03-20 21:06 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (8.85 KB, patch)
2012-03-20 21:34 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (10.71 KB, patch)
2012-03-21 12:06 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (9.80 KB, patch)
2012-03-21 14:55 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (11.84 KB, patch)
2012-03-21 15:48 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (13.38 KB, patch)
2012-03-21 19:26 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (14.49 KB, patch)
2012-03-21 23:19 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (13.84 KB, patch)
2012-03-22 08:24 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (13.71 KB, patch)
2012-03-22 11:46 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (13.19 KB, patch)
2012-03-23 10:10 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (14.26 KB, patch)
2012-03-23 10:21 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (14.26 KB, patch)
2012-03-23 11:00 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (14.50 KB, patch)
2012-03-23 11:53 PDT, Dave Michael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Michael 2012-03-06 10:30:18 PST
This bug has been spotted before, but the fix didn't cover all cases:
https://bugs.webkit.org/show_bug.cgi?id=66181

HTMLPluginElement::getNPObject looks like this:
"""
    ASSERT(document()->frame());
    if (!m_NPObject)
        m_NPObject = document()->frame()->script()->createScriptObjectForPluginElement(this);
    return m_NPObject;
"""
After this point, the NPObject (via the relevant JavaScript bindings) holds a reference on the HTMLPluginElement, and the HTMLPluginElement also owns a reference on the NPObject that wraps it. This is a circular reference, so without intervention, the HTMLPluginElement will never be destroyed.

(Probably, getNPObject should have just passed its NPObject reference to callers to avoid ever having. However, changing this behavior is probably too disruptive to existing code).

The patch in:
https://bugs.webkit.org/show_bug.cgi?id=66181
added:
"""
void HTMLPlugInElement::removedFromDocument()
{
#if ENABLE(NETSCAPE_PLUGIN_API)
    printf("removedFromDocument; m_NPObject: %p.\n", m_NPObject);
    if (m_NPObject) {
        _NPN_ReleaseObject(m_NPObject);
        m_NPObject = 0;
    }
#endif

    HTMLFrameOwnerElement::removedFromDocument();
}
"""
This way, when the plugin element is removed from the document, the circular reference is cleared and the HTMLPluginElement can be destroyed.

However, since removedFromDocument is not run on reload (nor, I think, on navigation), the HTMLPluginElement is still left in memory on reload or navigation. This tends to retain the associated Document object in memory.

I think the right approach (which wez suggested in his previous bug) is to release m_NPObject in detach(). I'll upload a patch soon, though I'm still struggling with coming up with a good way to write an automated test to reproduce this.
Comment 1 Dave Michael 2012-03-06 11:04:25 PST
Created attachment 130409 [details]
Patch
Comment 2 Dave Michael 2012-03-06 11:06:50 PST
Any tips on how to make an automated test for this would be appreciated. I don't think there's a way to do it with a layout test. I've been told there's a leak checker bot that might catch this if I construct a test that reproduces the leak, but I think that all the memory is still reachable, so I don't think a leak detection tool will see it.
Comment 3 Dave Michael 2012-03-09 11:54:03 PST
I've been struggling to find a way to test this. It's difficult for a few reasons:
- The PluginView is properly destroyed, and the plugin is thus "Deallocated" properly.
- All NPObjects are (apparently) force-released on Frame teardown, at which point the HTMLPluginElement will also be deleted. As a result, this doesn't show up as a leaked WebCoreNode

Due to these, there doesn't seem to be a way to test this using a layout test. (However, a lot of memory is wasted in practice while the process is running, because the Document that owns the HTMLPluginElement must persist as long as any of its children do. In Chromium, this can waste close to 1MB per reload for a very simple page.)

I think a test is possible if I do it as some kind of a unit test, where I have access to the internal/native WebKit classes. Then I can think of at least a couple of options:
(a) expose the Document weak pointer for use in the test. Then, I can:
  1) grab a weak pointer to the Document
  2) force a reload
  3) force garbage collection
  4) And ensure that the weak pointer's target document has been deallocated.
(b) Add Node's "liveNodeSet" to the debug build for the sake of my test. Then, add a "bool IsLiveNode(const Node*)" function, and:
  1) Save the raw pointer to the HTMLPluginElement
  2) force a reload
  3) force garbage collection
  4) Ensure that the raw pointer from 1) is not a "live" Node anymore.

However... this all sounds like a lot of work with some downsides (exposing currently-private data in (a), affecting performance of debug builds in (b)).

Would it make sense to maybe do this patch without a test? Or is there a better way to test it that I haven't thought of?
Comment 4 Hajime Morrita 2012-03-15 04:29:40 PDT
Comment on attachment 130409 [details]
Patch

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

r- due to the format issue.

I don't try this by myself. but it looks the test plugin has a "logDestroy" attribute which we can emit a log for plugin destruction with.
See LayoutTests/plugins/resources/geturlnotify-on-destroy.html for example.

> Source/WebCore/ChangeLog:1
> +2012-03-06  Dave Michael  <dmichael@chromium.org>

Please follow the format:

        Single line bug summary from Bugzilla
        https://bugs.webkit.org/show_bug.cgi?id=xxxx

        Reviewed by...

        Then long explanation follows....
Comment 5 Dave Michael 2012-03-15 08:57:53 PDT
Created attachment 132057 [details]
Patch
Comment 6 Dave Michael 2012-03-15 09:07:46 PDT
(In reply to comment #4)
> (From update of attachment 130409 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130409&action=review
> 
> r- due to the format issue.
> 
> I don't try this by myself. but it looks the test plugin has a "logDestroy" attribute which we can emit a log for plugin destruction with.
> See LayoutTests/plugins/resources/geturlnotify-on-destroy.html for example.
I can do that, but unfortunately it will not exercise this bug. The render tree is getting destroyed properly, so the PluginView still calls "PluginPackage::unload", and that's the part of the code that tells the plugin to "Deallocate". So from the plugin's perspective, everything is working fine.

What is being improperly retained is the HTMLPluginElement itself in the DOM tree (and therefore its owning Document stays in memory as well).

I mentioned a couple of ideas above for possibly testing this by checking that the HTMLPluginElement Node (or Document) stays in memory longer than expected. But these approaches have drawbacks. Alternatively, it looks like the WebInspector also has a count of Nodes. If I can reach that from a Layout test, I might be able to test the problem that way.

> 
> > Source/WebCore/ChangeLog:1
> > +2012-03-06  Dave Michael  <dmichael@chromium.org>
> 
> Please follow the format:
> 
>         Single line bug summary from Bugzilla
>         https://bugs.webkit.org/show_bug.cgi?id=xxxx
> 
>         Reviewed by...
> 
>         Then long explanation follows....
Comment 7 Dave Michael 2012-03-15 14:05:53 PDT
Comment on attachment 132057 [details]
Patch

Thanks for the review. I updated the changelog and tried to explain why there's no test.
Comment 8 Hajime Morrita 2012-03-19 16:51:18 PDT
(In reply to comment #7)
> (From update of attachment 132057 [details])
> Thanks for the review. I updated the changelog and tried to explain why there's no test.
Got the point. I was confused.
Comment 9 Eric Seidel (no email) 2012-03-20 14:34:10 PDT
Comment on attachment 132057 [details]
Patch

Since we do not believe it possible to write an automated test, I think it would make sense to add a manual test.  My understanding is a very very simple page should trigger this.

Your manual test would go in "ManualTests" (in the root of the repository) and would basically instruct the user to reload the page repeatedly, and see if memory usage increases.

Please re-post wiht a manual test, and I'm ahppy to r+.
Comment 10 Dave Michael 2012-03-20 21:06:44 PDT
Created attachment 132963 [details]
Patch
Comment 11 Philippe Normand 2012-03-20 21:11:25 PDT
Comment on attachment 132963 [details]
Patch

Attachment 132963 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12072144
Comment 12 Build Bot 2012-03-20 21:27:41 PDT
Comment on attachment 132963 [details]
Patch

Attachment 132963 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12066170
Comment 13 Dave Michael 2012-03-20 21:34:17 PDT
Created attachment 132967 [details]
Patch
Comment 14 Dave Michael 2012-03-20 21:43:30 PDT
Comment on attachment 132967 [details]
Patch

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

eseidel: Thanks for the suggestion of adding capabilities to window.internals. This turned out to be easier than expected, so here's my first stab at a test using this. I basically just reload and make sure that there's only 1 document.

> LayoutTests/plugins/netscape-dom-access-and-reload.html:36
> +        if (numberOfLiveDocuments == 1)

I'm worried about ports that don't build WebInspector (do we have any? Mobile stuff, or headless ports maybe?)

In those cases, this test isn't really valid, and numberOfLiveDocuments will be 0. What's the best way of dealing with that?
Comment 15 Build Bot 2012-03-20 21:53:28 PDT
Comment on attachment 132967 [details]
Patch

Attachment 132967 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12066180
Comment 16 Build Bot 2012-03-20 22:00:31 PDT
Comment on attachment 132967 [details]
Patch

Attachment 132967 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12072159
Comment 17 Philippe Normand 2012-03-20 22:38:26 PDT
Comment on attachment 132967 [details]
Patch

Attachment 132967 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12066204
Comment 18 Adam Barth 2012-03-21 10:47:46 PDT
Comment on attachment 132967 [details]
Patch

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

>> LayoutTests/plugins/netscape-dom-access-and-reload.html:36
>> +        if (numberOfLiveDocuments == 1)
> 
> I'm worried about ports that don't build WebInspector (do we have any? Mobile stuff, or headless ports maybe?)
> 
> In those cases, this test isn't really valid, and numberOfLiveDocuments will be 0. What's the best way of dealing with that?

That's fine.  Those ports (if they exist) can skip this test.  Perhaps we should show a helpful message if numberOfLiveDocuments == 0 letting folks know that this might be caused by having the inspector disabled?
Comment 19 Dave Michael 2012-03-21 12:06:49 PDT
Created attachment 133088 [details]
Patch
Comment 20 Build Bot 2012-03-21 12:42:44 PDT
Comment on attachment 133088 [details]
Patch

Attachment 133088 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12071570
Comment 21 Build Bot 2012-03-21 12:56:36 PDT
Comment on attachment 133088 [details]
Patch

Attachment 133088 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12090539
Comment 22 Philippe Normand 2012-03-21 14:24:19 PDT
Comment on attachment 133088 [details]
Patch

Attachment 133088 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12072595
Comment 23 Dave Michael 2012-03-21 14:55:02 PDT
Created attachment 133118 [details]
Patch
Comment 24 Dave Michael 2012-03-21 15:31:10 PDT
+dglazkov, caseq: Can one of you guys help me figure out how to get this to build properly? I'm trying to use InspectorCounters::counterValue in testing/Internals.cpp, and I'm getting link errors. I might have mac sorted out (I added the symbol to WebCore.exp.in), but I'm not sure where in WebKit to tell the Windows build to export that symbol. GTK is also failing to build (though it's not clear to me if that's related to my patch)
Comment 25 Build Bot 2012-03-21 15:33:02 PDT
Comment on attachment 133118 [details]
Patch

Attachment 133118 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12066633
Comment 26 Dave Michael 2012-03-21 15:48:20 PDT
Created attachment 133132 [details]
Patch
Comment 27 Philippe Normand 2012-03-21 15:54:24 PDT
./.libs/libWebCoreInternals.a(./.libs/../Source/WebCore/testing/.libs/libWebCoreInternals_la-Internals.o):Internals.cpp:function WebCore::Internals::numberOfLiveNodes() const: error: undefined reference to 'WebCore::InspectorCounters::counterValue(WebCore::InspectorCounters::CounterType)'
./.libs/libWebCoreInternals.a(./.libs/../Source/WebCore/testing/.libs/libWebCoreInternals_la-Internals.o):Internals.cpp:function WebCore::Internals::numberOfLiveDocuments() const: error: undefined reference to 'WebCore::InspectorCounters::counterValue(WebCore::InspectorCounters::CounterType)'
collect2: ld returned 1 exit status

You need to add those 2 symbols in Source/autotools/symbols.filter
Comment 28 Gustavo Noronha (kov) 2012-03-21 15:58:26 PDT
Comment on attachment 133132 [details]
Patch

Attachment 133132 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12072631
Comment 29 Dave Michael 2012-03-21 19:26:06 PDT
Created attachment 133170 [details]
Patch
Comment 30 Dave Michael 2012-03-21 20:50:13 PDT
Comment on attachment 133170 [details]
Patch

I think I have all the right incantations to make this build on all the platforms.
Comment 31 Ryosuke Niwa 2012-03-21 21:36:11 PDT
Comment on attachment 133170 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the description but after the bug summary and the bug url.

> Source/WebCore/html/HTMLPlugInElement.cpp:83
>          _NPN_ReleaseObject(m_NPObject);

Can this trigger a layout or style recalc? i.e. is the np object gets notified synchronously here? If so, it's not safe to do this.

> Source/WebCore/testing/Internals.idl:125
> +        unsigned long numberOfLiveNodes();
> +        unsigned long numberOfLiveDocuments();

It seems like we should just wrap this inside ENABLE(INSPECTOR).
It's misleading for it to return 0. Note that the right way to do that is appending [Conditional=INSPECTOR] on each function declaration.

> LayoutTests/plugins/netscape-dom-access-and-reload.html:10
> +    // Make the plugin retrieve a DOM element to itself. This exercises
> +    // https://bugs.webkit.org/show_bug.cgi?id=80428
> +    // (HTMLPluginElement is not destroyed on reload or navigation if
> +    // getNPObject is called)

This comment is redundant since it's included in the body.

> LayoutTests/plugins/netscape-dom-access-and-reload.html:13
> +    // Reload the page once.

This comment repeats the code. Please remove.

> LayoutTests/plugins/netscape-dom-access-and-reload.html:28
> +        window.location.reload();

No need for "window.".

> LayoutTests/plugins/netscape-dom-access-and-reload.html:35
> +        var numberOfLiveDocuments = window.internals.numberOfLiveDocuments();

No need for "window.".

> LayoutTests/plugins/netscape-dom-access-and-reload.html:41
> +                "FAILED; numberOfLiveDocuments returned 0. This test is only "
> +                "valid if the Inspector is enabled.";

If we had made numberOfLiveDocuments conditional, you can detect this in the script.
Comment 32 Dave Michael 2012-03-21 23:19:11 PDT
Created attachment 133193 [details]
Patch
Comment 33 Dave Michael 2012-03-21 23:23:24 PDT
Comment on attachment 133170 [details]
Patch

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

>> Source/WebCore/html/HTMLPlugInElement.cpp:83
>>          _NPN_ReleaseObject(m_NPObject);
> 
> Can this trigger a layout or style recalc? i.e. is the np object gets notified synchronously here? If so, it's not safe to do this.

I'm not certain if it's possible for the plugin to cause a layout from its NPP_Deallocate function (it may be prohibited). But it's scary enough that I used a Timer to defer the release.

(Good catch)

>> Source/WebCore/testing/Internals.idl:125
>> +        unsigned long numberOfLiveDocuments();
> 
> It seems like we should just wrap this inside ENABLE(INSPECTOR).
> It's misleading for it to return 0. Note that the right way to do that is appending [Conditional=INSPECTOR] on each function declaration.

Good idea. Done.

>> LayoutTests/plugins/netscape-dom-access-and-reload.html:10
>> +    // getNPObject is called)
> 
> This comment is redundant since it's included in the body.

Removed.

>> LayoutTests/plugins/netscape-dom-access-and-reload.html:13
>> +    // Reload the page once.
> 
> This comment repeats the code. Please remove.

I removed it. I guess I thought it was helpful since it summarizes the purpose of the next 7 lines of code.

>> LayoutTests/plugins/netscape-dom-access-and-reload.html:28
>> +        window.location.reload();
> 
> No need for "window.".

removed.

>> LayoutTests/plugins/netscape-dom-access-and-reload.html:35
>> +        var numberOfLiveDocuments = window.internals.numberOfLiveDocuments();
> 
> No need for "window.".

removed.

>> LayoutTests/plugins/netscape-dom-access-and-reload.html:41
>> +                "valid if the Inspector is enabled.";
> 
> If we had made numberOfLiveDocuments conditional, you can detect this in the script.

Done; this is cleaner!
Comment 34 Dimitri Glazkov (Google) 2012-03-22 08:01:35 PDT
Comment on attachment 133193 [details]
Patch

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

> Source/WebCore/html/HTMLPlugInElement.cpp:86
> +    m_releaseNPObjectTimer.startOneShot(0);

Won't queuePostAttachCallback work instead? Seems like it was designed for this purpose.

> LayoutTests/plugins/netscape-dom-access-and-reload.html:38
> +              document.getElementById("result").innerHTML = "FAILED; This test is only valid if the Inspector is enabled.";

Inspector _and_ DumpRenderTree, right?
Comment 35 Dave Michael 2012-03-22 08:24:20 PDT
Created attachment 133269 [details]
Patch
Comment 36 Dave Michael 2012-03-22 08:33:18 PDT
Comment on attachment 133193 [details]
Patch

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

Thanks for looking! I uploaded a new patch.

>> Source/WebCore/html/HTMLPlugInElement.cpp:86
>> +    m_releaseNPObjectTimer.startOneShot(0);
> 
> Won't queuePostAttachCallback work instead? Seems like it was designed for this purpose.

Perhaps... that sounds like it's meant for stuff to do just after attach, rather than detach? But in any case, I just didn't know about it :-)

Turns out, though, that I was just not thinking straight when going over rniwa's comments. Releasing HTMLPlugInElement doesn't give the plugin an opportunity to do anything. So I changed it back to just releasing immediately, and I added an explanation to the ChangeLog (as suggested by rniwa on IRC).

>> LayoutTests/plugins/netscape-dom-access-and-reload.html:38
>> +              document.getElementById("result").innerHTML = "FAILED; This test is only valid if the Inspector is enabled.";
> 
> Inspector _and_ DumpRenderTree, right?

Done
Comment 37 Dimitri Glazkov (Google) 2012-03-22 09:30:56 PDT
Comment on attachment 133269 [details]
Patch

Looks good. I would like Dr. Seidel to give it one more look.
Comment 38 Eric Seidel (no email) 2012-03-22 11:40:52 PDT
Comment on attachment 133269 [details]
Patch

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

LGTM.

> Source/WebCore/html/HTMLPlugInElement.h:30
> +#include "Timer.h"

Why is this needed?
Comment 39 Dave Michael 2012-03-22 11:46:22 PDT
Created attachment 133307 [details]
Patch
Comment 40 Eric Seidel (no email) 2012-03-22 12:05:35 PDT
Comment on attachment 133307 [details]
Patch

Fantastic!
Comment 41 WebKit Review Bot 2012-03-22 13:45:53 PDT
Comment on attachment 133307 [details]
Patch

Clearing flags on attachment: 133307

Committed r111754: <http://trac.webkit.org/changeset/111754>
Comment 42 WebKit Review Bot 2012-03-22 13:46:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 43 mitz 2012-03-22 23:25:43 PDT
(In reply to comment #41)
> (From update of attachment 133307 [details])
> Clearing flags on attachment: 133307
> 
> Committed r111754: <http://trac.webkit.org/changeset/111754>

The test added in this change has been failing in Mac WebKit2 ever since. Filed bug 82020.
Comment 44 Ryosuke Niwa 2012-03-23 00:54:54 PDT
I think this patch caused plugins/reloadplugins-and-pages.html to fail on all platforms:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=plugins%2Freloadplugins-and-pages.html
Comment 45 Ryosuke Niwa 2012-03-23 00:59:39 PDT
Looking at http://trac.webkit.org/log/?verbose=on&rev=111757&stop_rev=111749, I'm certain this patch caused the failure.

Given that the test added by this patch is failing on Mac WebKit 2 and it caused a regression, I'm going to rollout the patch along with mitz's WK2 rebaseline in the next half an hour or so. Please let me know ASAP if you know how to fix it such that we don't have to roll out the patch.
Comment 46 Ryosuke Niwa 2012-03-23 02:07:18 PDT
Reopen the bug since the patch was rolled out in http://trac.webkit.org/changeset/111839.
Comment 47 Ryosuke Niwa 2012-03-23 02:10:15 PDT
http://crbug.com/114023/
Comment 48 Dave Michael 2012-03-23 10:10:40 PDT
Created attachment 133506 [details]
Patch
Comment 49 Dave Michael 2012-03-23 10:21:25 PDT
Created attachment 133507 [details]
Patch
Comment 50 Ryosuke Niwa 2012-03-23 10:41:27 PDT
Comment on attachment 133507 [details]
Patch

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

> LayoutTests/ChangeLog:11
> +        Due to unfortunate copy/paste laziness, the new test was using the same
> +        window.sessionStorage as plugins/reloadplugins-and-pages.html, so that
> +        if the tests were run in the same session, reloadplugins-and-pages.html
> +        would *not* reload as it was supposed to, causing a text mismatch. This
> +        patch uses a more appropriate and unique name so that these two tests
> +        won't affect each other.

Isn't this a DRT bug? It seems like we should be re-setting the session storage whenever new test is loaded.

e.g. Mac port has:
http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/mac/DumpRenderTree.mm#L1225

> LayoutTests/ChangeLog:13
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the description.
Comment 51 Dave Michael 2012-03-23 11:00:32 PDT
Created attachment 133512 [details]
Patch
Comment 52 Dave Michael 2012-03-23 11:06:24 PDT
Comment on attachment 133507 [details]
Patch

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

>> LayoutTests/ChangeLog:11
>> +        won't affect each other.
> 
> Isn't this a DRT bug? It seems like we should be re-setting the session storage whenever new test is loaded.
> 
> e.g. Mac port has:
> http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/mac/DumpRenderTree.mm#L1225

I wouldn't know. I filed a bug: https://bugs.webkit.org/show_bug.cgi?id=82068

Hopefully we can deal with that separately?

>> LayoutTests/ChangeLog:13
>> +        Reviewed by NOBODY (OOPS!).
> 
> This line should appear before the description.

Augh, someday I'll get the ChangeLog stuff right!

Thanks, Done :-)
Comment 53 Ryosuke Niwa 2012-03-23 11:34:30 PDT
Comment on attachment 133512 [details]
Patch

Forwarding eric's r+. (You should modify Reviewed by line to include both names of our names if you're so inclined to include my name).
Comment 54 Dave Michael 2012-03-23 11:53:09 PDT
Created attachment 133528 [details]
Patch
Comment 55 WebKit Review Bot 2012-03-23 13:00:27 PDT
Comment on attachment 133528 [details]
Patch

Clearing flags on attachment: 133528

Committed r111890: <http://trac.webkit.org/changeset/111890>
Comment 56 WebKit Review Bot 2012-03-23 13:00:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 57 mitz 2012-03-23 17:18:16 PDT
(In reply to comment #55)
> (From update of attachment 133528 [details])
> Clearing flags on attachment: 133528
> 
> Committed r111890: <http://trac.webkit.org/changeset/111890>

The test added in this change has been failing in Mac WebKit2 ever since. This is the second time a change for this bug does this. Please keep an eye on build.webkit.org after committing a change to see if any new tests are failing.