[webkit-reviews] review denied: [Bug 53611] Propagate security origin of parent document into HTML documents created with DOMImplementation : [Attachment 81047] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied	review:
Bug 53611: Propagate security origin of parent document into HTML documents
created with DOMImplementation
https://bugs.webkit.org/show_bug.cgi?id=53611

Attachment 81047: Patch
https://bugs.webkit.org/attachment.cgi?id=81047&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81047&action=review

OK, I took a deeper look now, and I'm puzzled why this has r+ at all. I would
encourage reviewers to pay more attention to what they approve.

> Source/WebCore/ChangeLog:10
> +	   Absence of regressions is covered by the current tests.  It's
difficult
> +	   to check this via layout tests as objects reside in the same JS
context and hence
> +	   there are no security origin checks.

If this is not observable, why do this at all? And if it's just difficult to
test, it still needs a test.

Note that we already have code to deal with security context of documents
created via DOMImplementation in Document::initSecurityContext(). If it's
wrong, or the comment there is wrong, that should be addressed.

Finally, this change directly contradicts DOM 3 Core
<http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-102161490>, which says "The
DOMImplementation interface provides a number of methods for performing
operations that are independent of any particular instance of the document
object model." There should be an explanation of why that's OK.

> Source/WebCore/dom/DOMImplementation.cpp:174
> +DOMImplementation::DOMImplementation(const Document* parent) :
m_parent(parent)

Initialization should be on a separate line.

> Source/WebCore/dom/DOMImplementation.cpp:251
> +    if (!m_parent) {
> +	   ec = INVALID_STATE_ERR;
> +	   return 0;
> +    }

Is this how it works in other browsers? This is definitely testable, and there
should be a regression test.

Garbage collection should never be observable in JavaScript, so instead of
checking for document having been destroyed and raising an exception, you
should make sure that it is not destroyed while observable through
DOMImplementation. For JSC, this is done by overriding markChildren method (see
e.g. JSDocument::markChildren() in JSDocumentCustom.cpp).

> Source/WebCore/dom/DOMImplementation.h:68
> +    void documentDestroyed()
> +    {
> +	   m_parent = 0;
> +    }

This should be on one line.

> Source/WebCore/dom/DOMImplementation.h:73
> +    const Document* m_parent;

The document is not a parent of DOMImplementation, so the name is misleading.