Bug 24142 - Add test for custom DOM properties surviving garbage collection
Summary: Add test for custom DOM properties surviving garbage collection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pam Greene (IRC:pamg)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-24 16:10 PST by Pam Greene (IRC:pamg)
Modified: 2009-03-04 12:59 PST (History)
0 users

See Also:


Attachments
New test + result (3.70 KB, patch)
2009-02-24 16:16 PST, Pam Greene (IRC:pamg)
fishd: review-
Details | Formatted Diff | Diff
Patch addressing Darin's comments (3.60 KB, patch)
2009-02-25 07:37 PST, Pam Greene (IRC:pamg)
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pam Greene (IRC:pamg) 2009-02-24 16:10:35 PST
Ensure that custom DOM properties (in widow) are not lost during garbage collection.

Browsers differ in whether they keep properties attached to various window properties, but their behavior for window.navigator and window.location is consistent.

Also see http://code.google.com/p/chromium/issues/detail?id=1994 for a more detailed analysis.
Comment 1 Pam Greene (IRC:pamg) 2009-02-24 16:16:19 PST
Created attachment 27942 [details]
New test + result

This expected result follows the current ToT behavior on Mac. The result for Safari 3.2.1 on Windows is different, but I couldn't find any bug discussion relating to a change.

In practice, since the behavior of everything except window.location.foo and window.navigator.foo varies among browsers (at least for now), simply protecting against unintended changes is enough.
Comment 2 Darin Fisher (:fishd, Google) 2009-02-24 22:51:44 PST
Comment on attachment 27942 [details]
New test + result

>Index: fast/dom/Window/customized-property-survives-gc.html
...
>+function CollectGarbage() {
>+  if (window.GCController) {
>+    // This method is not available in Chromium, only in its test_shell.

It's also true that window.GCController is undefined in Safari.  From the
perspective of the WebKit project, it may be better to just say that this
method is only available when executed from DumpRenderTree.  That said,
it may not really be worth commenting here since other similar tests do
not have these kinds of comments.


>+function check(name, should_survive) {
>+  window[name].myProp = 10;
>+  CollectGarbage();
>+  if (should_survive)

nit: should_survive -> shouldSurvive
Comment 3 Alexey Proskuryakov 2009-02-25 06:47:51 PST
Good tests work in browser, too - see e.g. fast/workers/worker-gc.html for a technique that we use. Decent tests do print a warning.
Comment 4 Pam Greene (IRC:pamg) 2009-02-25 07:37:54 PST
Created attachment 27965 [details]
Patch addressing Darin's comments

Adjusted the comment on CollectGarbage() and renamed should_survive.

(In reply to comment #3)
> Good tests work in browser, too - see e.g. fast/workers/worker-gc.html for a
> technique that we use. Decent tests do print a warning.

Sorry, but I don't follow what you're requesting. This test uses the same technique of allocating objects, which works in Safari and Firefox but not Chromium (not V8, really); I don't know about other JS engines. It also prints a form of warning, up in the test description: "Results may be different when run manually...."  Is more than that needed?
Comment 5 Darin Fisher (:fishd, Google) 2009-02-25 15:50:46 PST
Comment on attachment 27965 [details]
Patch addressing Darin's comments

LGTM

It is true that we don't have a mechanism to reliably force GC within Chromium due to V8's generational GC.
Comment 6 Pam Greene (IRC:pamg) 2009-03-04 12:59:27 PST
Landed as r41254.