Bug 164004 – [DOMJIT] Implement Node::ownerDocument

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

I figured out that it is called in jQuery isXMLDoc function.
And it is also called from jQuery.prop() function. And it seems that this prop() is the hottest jQuery function in Ember.js SpeedoMeter.

[Yusuke Suzuki]

Good things:

Looking up the ownerDocument from the given Node is also done in CSS JIT.
So there is already a function like xxxMemoryOffset(). So the difficulty is super low.
And according to the inspector team, it is frequently called in the inspector.

[Yusuke Suzuki]

Created attachment 292910 [details]
Patch

[Darin Adler]

Comment on attachment 292910 [details]
Patch

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

> Source/WebCore/domjit/JSNodeDOMJIT.cpp:191
> +// Node#ownerDocument

I don’t think these comments are helpful. The class names already contain this information.

[Yusuke Suzuki]

Comment on attachment 292910 [details]
Patch

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

Thanks!

>> Source/WebCore/domjit/JSNodeDOMJIT.cpp:191
>> +// Node#ownerDocument
> 
> I don’t think these comments are helpful. The class names already contain this information.

Yeah, OK, I'll drop them :)

[Yusuke Suzuki]

Committed r207932: <http://trac.webkit.org/changeset/207932>

[Yusuke Suzuki]

jQuery.attr operation is accelerated and shows 15% improvement in Dromaeo jslib-attr-jquery's jQuery.attr(class).

[Sam Weinig]

(In reply to comment #6)
> jQuery.attr operation is accelerated and shows 15% improvement in Dromaeo
> jslib-attr-jquery's jQuery.attr(class).

That's awesome.

[Yusuke Suzuki]

It causes segv. rolled out.
Investigating.
I guess document() may become null while it is represented as a reference at some point (maybe, when leaving a page).

[Yusuke Suzuki]

hmmmmmmm, still I cannot reproduce it on x64 environment.
The crash log said that we crash with 0x8 access. It is apparently nullptr + offset access.

I think the most suspicious part case is "document becomes nullptr". We access the document + ScriptWrappable::offsetOfWrapper<Document>(). And ScriptWrappable::offsetOfWrapper<Document>() is 8. If it returns nullptr, it should cause the segv with 0x8 address in this code.

And C++ ownerDocument() does not cause this crash because the C++ code is looks like this.

Document* document = &this->document();
return document == this ? nullptr : document;

If document becomes nullptr, it does not cause any crash. It just returns nullptr.
For `document == this` part, I used `branchIfNotDocumentWrapper`. But if it may becomes nullptr, we should organize the JIT code as strictly corresponding to C++ code. (And it should be easy).

The problematic part is document() is Document&.

[Yusuke Suzuki]

Created attachment 293865 [details]
Patch

[Yusuke Suzuki]

(In reply to comment #10)
> Created attachment 293865 [details]
> Patch

Still I cannot reproduce it in x64 environment & I'm still setting up the ARM64 environment, I believe this should fix the crash.
Still investigating...

[Yusuke Suzuki]

Created attachment 293866 [details]
Patch

[Yusuke Suzuki]

Finally, I've reproduced in arm64. Investigating...

[Yusuke Suzuki]

(In reply to comment #13)
> Finally, I've reproduced in arm64. Investigating...

OK, I reproduced the issue with the previous version.
And I've ensured this patch fixes the issue.
I'll ensure that sometimes `document()` returns nullptr next.

[Yusuke Suzuki]

(In reply to comment #14)
> (In reply to comment #13)
> > Finally, I've reproduced in arm64. Investigating...
> 
> OK, I reproduced the issue with the previous version.
> And I've ensured this patch fixes the issue.
> I'll ensure that sometimes `document()` returns nullptr next.

I've figured out the cause of this bug. The above guess is wrong.
In C++ ownerDocument() code, we have like,

    Document* document = &this->document();
    return document == this ? nullptr : document;

Instead, DOMJIT ownerDocument used DOMJIT::branchIfDocumentWrapper() to check if the given object is Document.
But, interestingly, it seems that there is some objects, that nodeType embed in JSType is not DocumentWrapper, but document() == this.

[Yusuke Suzuki]

Finally I figured out the issue.
It is not directly related to this patch. MacroAssembler's bug.
https://bugs.webkit.org/show_bug.cgi?id=164432

[Darin Adler]

Comment on attachment 293866 [details]
Patch

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

> Source/WebCore/domjit/JSNodeDOMJIT.cpp:180
> +        RELEASE_ASSERT(!CAST_OFFSET(EventTarget*, Node*));
> +        RELEASE_ASSERT(!CAST_OFFSET(Node*, Document*));

Can we use a static_assert here instead of RELEASE_ASSERT?

> Source/WebCore/domjit/JSNodeDOMJIT.cpp:185
> +        // In C++ ownerDocument implementation, if &document() becomes nullptr, it just returns nullptr.
> +        // DOMJIT implementation strictly follows the C++ one.
> +        nullCases.append(jit.branchTestPtr(CCallHelpers::Zero, document));

We need to look into this more. It is annoying that we have to add a branch for this, and even the C++ implementation would crash if we compiled it with -fcatch-undefined-behavior and the document reference was null. We should reproduce the problem, fix it, and remove this branch!

[Yusuke Suzuki]

Comment on attachment 293866 [details]
Patch

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

Thanks!

>> Source/WebCore/domjit/JSNodeDOMJIT.cpp:185
>> +        nullCases.append(jit.branchTestPtr(CCallHelpers::Zero, document));
> 
> We need to look into this more. It is annoying that we have to add a branch for this, and even the C++ implementation would crash if we compiled it with -fcatch-undefined-behavior and the document reference was null. We should reproduce the problem, fix it, and remove this branch!

Oops! I should update this patch. Sorry. I set up my device and finally figured out the issue :D
And it is not related to this patch. Our MacroAssembler in non x86 has a bug with 8bit operations (like branch8). And this is why x86 Safari does not pose any problems.
See the comment in https://bugs.webkit.org/show_bug.cgi?id=164004#c16.
And https://bugs.webkit.org/show_bug.cgi?id=164432 is handling this issue.

Once the macro assembler issue is fixed, we can land the reverted patch as is.
At that time, I'll insert assertions newly added in this patch :)

[Yusuke Suzuki]

Comment on attachment 293866 [details]
Patch

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

>> Source/WebCore/domjit/JSNodeDOMJIT.cpp:180
>> +        RELEASE_ASSERT(!CAST_OFFSET(Node*, Document*));
> 
> Can we use a static_assert here instead of RELEASE_ASSERT?

Because CAST_OFFSET uses reinterpret_cast, it is not used in the context of static_assert, :(. That's sad thing.
(And OBJECT_OFFSETOF macro is the same).

[Yusuke Suzuki]

Comment on attachment 293866 [details]
Patch

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

>>> Source/WebCore/domjit/JSNodeDOMJIT.cpp:185
>>> +        nullCases.append(jit.branchTestPtr(CCallHelpers::Zero, document));
>> 
>> We need to look into this more. It is annoying that we have to add a branch for this, and even the C++ implementation would crash if we compiled it with -fcatch-undefined-behavior and the document reference was null. We should reproduce the problem, fix it, and remove this branch!
> 
> Oops! I should update this patch. Sorry. I set up my device and finally figured out the issue :D
> And it is not related to this patch. Our MacroAssembler in non x86 has a bug with 8bit operations (like branch8). And this is why x86 Safari does not pose any problems.
> See the comment in https://bugs.webkit.org/show_bug.cgi?id=164004#c16.
> And https://bugs.webkit.org/show_bug.cgi?id=164432 is handling this issue.
> 
> Once the macro assembler issue is fixed, we can land the reverted patch as is.
> At that time, I'll insert assertions newly added in this patch :)

So, Ive just dropped this check. The macro assembler fix should address this issue.

[Yusuke Suzuki]

Committed r208481: <http://trac.webkit.org/changeset/208481>