Make DOMParser create documents with a context document set

Make DOMParser create documents with a context document set

Store a (weak) reference to the current context's document in DOMParser
objects when created, and use this in parseFromString() when creating
the parsed document.

This matches how parsed documents are created by XMLHttpRequest.

BUG=456624
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189959

[Jens Widell, 2015-02-11 07:53:00]

jl@opera.com changed reviewers:
+ haraken@chromium.org

[Jens Widell, 2015-02-11 07:53:01]

PTAL

I just had a quick look at this issue. If you have another fix or plan, I'll
happily drop this.

https://codereview.chromium.org/919473004/diff/1/Source/core/xml/DOMParser.h
File Source/core/xml/DOMParser.h (right):

https://codereview.chromium.org/919473004/diff/1/Source/core/xml/DOMParser.h#...
Source/core/xml/DOMParser.h:47: WeakPtrWillBeWeakMember<Document>
m_contextDocument;
I'm not exactly familiar with this particular flavor of reference, but it seems
to be the pattern elsewhere.

[haraken, 2015-02-11 08:23:11]

Thanks for looking at this!

Can we add a test?

https://codereview.chromium.org/919473004/diff/1/Source/core/xml/DOMParser.cpp
File Source/core/xml/DOMParser.cpp (right):

https://codereview.chromium.org/919473004/diff/1/Source/core/xml/DOMParser.cp...
Source/core/xml/DOMParser.cpp:30: RefPtrWillBeRawPtr<Document> doc =
DOMImplementation::createDocument(type, DocumentInit(KURL(), nullptr,
m_contextDocument), false);

What happens if m_contextDocument is already cleared out at this point?

[Jens Widell, 2015-02-11 08:29:07]

On 2015/02/11 08:23:11, haraken wrote:
> Thanks for looking at this!
> 
> Can we add a test?

Yes, I'll add a test.

https://codereview.chromium.org/919473004/diff/1/Source/core/xml/DOMParser.cpp
File Source/core/xml/DOMParser.cpp (right):

https://codereview.chromium.org/919473004/diff/1/Source/core/xml/DOMParser.cp...
Source/core/xml/DOMParser.cpp:30: RefPtrWillBeRawPtr<Document> doc =
DOMImplementation::createDocument(type, DocumentInit(KURL(), nullptr,
m_contextDocument), false);
On 2015/02/11 08:23:11, haraken wrote:
> 
> What happens if m_contextDocument is already cleared out at this point?

Same thing as before, i.e. we crash if we parse a marquee element. We could
disable parseFromString() then, but I'm not sure that's a good thing.

We could also just disable the private JS compilation in HTMLMarqueeElement() if
there's no context document (or remove it entirely?). AFAICT the private JS is
compiled if any attribute is accessed on the element instead then, and thus
things still work.

[Jens Widell, 2015-02-11 09:05:47]

On 2015/02/11 08:29:07, Jens Widell wrote:
> On 2015/02/11 08:23:11, haraken wrote:
> > Thanks for looking at this!
> > 
> > Can we add a test?
> 
> Yes, I'll add a test.

Test added.

[haraken, 2015-02-11 09:12:57]

On 2015/02/11 08:29:07, Jens Widell wrote:
> On 2015/02/11 08:23:11, haraken wrote:
> > Thanks for looking at this!
> > 
> > Can we add a test?
> 
> Yes, I'll add a test.
> 
> https://codereview.chromium.org/919473004/diff/1/Source/core/xml/DOMParser.cpp
> File Source/core/xml/DOMParser.cpp (right):
> 
>
https://codereview.chromium.org/919473004/diff/1/Source/core/xml/DOMParser.cp...
> Source/core/xml/DOMParser.cpp:30: RefPtrWillBeRawPtr<Document> doc =
> DOMImplementation::createDocument(type, DocumentInit(KURL(), nullptr,
> m_contextDocument), false);
> On 2015/02/11 08:23:11, haraken wrote:
> > 
> > What happens if m_contextDocument is already cleared out at this point?
> 
> Same thing as before, i.e. we crash if we parse a marquee element. We could
> disable parseFromString() then, but I'm not sure that's a good thing.
> 
> We could also just disable the private JS compilation in HTMLMarqueeElement()
if
> there's no context document (or remove it entirely?). AFAICT the private JS is
> compiled if any attribute is accessed on the element instead then, and thus
> things still work.

Yeah, you're right. This will work for the private JS case. I have no idea for a
more general fix, at least, this CL is definitely an improvement. LGTM.

[Jens Widell, 2015-02-11 09:21:25]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2015-02-11 09:21:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/919473004/20001

[commit-bot: I haz the power, 2015-02-11 10:39:01]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189959