DOMWindow constructor directly callable

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Michael Saboff]

This defect is a result of tracking down <rdar://problem/8281342>.

The original problem is that Safari SPODs on most mouse events on direct.tesco.com.

The problem was tracked down to a deep copy implementation that starts with the code
    var objectClone = new this.constructor()
Other browsers disallow calls to DOMWindow.constructor() while webcore defaults to the super class constructor for Object.

There are other issues with the deep copy javascript method specifically not handling reference loops which results in unbounded recursion (and thus the SPOD).  Disallowing direct calls to DOMWindow.constructor fixes the issue as well.

[Michael Saboff]

Created attachment 70296 [details]
Patch to add DOMWindow constructor

This patch adds a constructor to DOMWindow objects.  That constructor is not directly callable in javascript.

Several tests were updated and one test was added.

A separate "patch" highlights the changes to generated code.

[Michael Saboff]

Created attachment 70297 [details]
This patch shows the changes to generated code.

This "patch" shows the changes to JSDOMWindow.cpp that result from https://bugs.webkit.org/attachment.cgi?id=70296

[Oliver Hunt]

Comment on attachment 70296 [details]
Patch to add DOMWindow constructor

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

Code changes look sane, just not 100% happy with the .constructor test case.

> LayoutTests/fast/dom/Window/window-constructor-expected.txt:1
> +FAIL: Timed out waiting for notifyDone to be called

This indicates that you're not calling notifyDone() when the test finishes -- but is it really necessary for this test to be an onload?

[Sam Weinig]

Comment on attachment 70296 [details]
Patch to add DOMWindow constructor

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

> LayoutTests/fast/dom/Window/window-constructor.html:6
> +    window.layoutTestController.waitUntilDone();

No need to call waitUntilDone() here.

> LayoutTests/fast/dom/Window/window-property-descriptors-expected.txt:500
> +FAIL Object.getOwnPropertyDescriptor(window, 'constructor') should be undefined. Was [object Object]

You should change this test to have the correct assumption about constructor.

[Michael Saboff]

Created attachment 70305 [details]
Patch updated with suggested test changes

This is the original patch with suggested test changes / simplifications.

[Alexey Proskuryakov]

There are still tests that changed from PASS to FAIL. If we believe that the new results are correct, expectations should be changed to keep tests passing.

[Michael Saboff]

Created attachment 70323 [details]
Patch with additional tests fixes

This patch fixes the other three tests that had new FAIL's which have been revised to PASSes.

[Darin Adler]

Comment on attachment 70323 [details]
Patch with additional tests fixes

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

Looks like we have good test coverage of this. Great fix.

> WebCore/WebCore.xcodeproj/project.pbxproj:-21153
> -			developmentRegion = English;

You are using the old Xcode?

> WebCore/bindings/scripts/CodeGeneratorJS.pm:2851
> -    push(@$outputArray, "    putDirect(exec->propertyNames().prototype, ${protoClassName}::self(exec, globalObject), DontDelete | ReadOnly);\n");
> +    if ($interfaceName eq "DOMWindow") {
> +        push(@$outputArray, "    putDirect(exec->propertyNames().prototype, globalObject->prototype(), DontDelete | ReadOnly);\n");
> +    } else {
> +        push(@$outputArray, "    putDirect(exec->propertyNames().prototype, ${protoClassName}::self(exec, globalObject), DontDelete | ReadOnly);\n");
> +    }

Seems like it would be slightly cleaner to just use a variable for the prototype expression. But I think this is OK as-is.

[WebKit Commit Bot]

Comment on attachment 70323 [details]
Patch with additional tests fixes

Rejecting patch 70323 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 70323]" exit_code: 1
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=70323&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=47422&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Updating working directory
Processing patch 70323 from bug 47422.
ERROR: /Projects/CommitQueue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/4342002

[Michael Saboff]

Created attachment 70447 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 70447 [details]
Patch

Rejecting patch 70447 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 70447]" exit_code: 2
Last 500 characters of output:
/ToObject-001.js
	M	WebCore/ChangeLog
	M	WebCore/bindings/scripts/CodeGeneratorJS.pm
	M	WebCore/page/DOMWindow.idl
A repository hook failed: Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/LayoutTests/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/git/libexec/git-core/git-svn line 572


Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1

Full output: http://queues.webkit.org/results/4304028

[Michael Saboff]

Created attachment 70454 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 70454 [details]
Patch

Clearing flags on attachment: 70454

Committed r69553: <http://trac.webkit.org/changeset/69553>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/69553 might have broken GTK Linux 64-bit Debug

[Csaba Osztrogonác]

(In reply to comment #15)
> http://trac.webkit.org/changeset/69553 might have broken GTK Linux 64-bit Debug

And Qt bot too ... :S

Qt specific expected results updated: http://trac.webkit.org/changeset/69562

[Adam Roben (:aroben)]

It looks like this broke Windows: http://build.webkit.org/results/Windows%20Release%20(Tests)/r69590%20(5075)/results.html

(The SVG failures are unrelated.)

[Alexey Proskuryakov]

*** Bug 44234 has been marked as a duplicate of this bug. ***