Bug 164004

Summary: [DOMJIT] Implement Node::ownerDocument
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dbates, esprehn+autocc, fpizlo, ggaren, joepeck, kangil.han, kondapallykalyan, saam, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 164432    
Bug Blocks: 162544, 164006    
Attachments:
Description Flags
Patch
none
Patch
none
Patch darin: review+

Description Yusuke Suzuki 2016-10-25 22:03:27 PDT
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.
Comment 1 Yusuke Suzuki 2016-10-25 22:05:00 PDT
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.
Comment 2 Yusuke Suzuki 2016-10-26 02:35:55 PDT
Created attachment 292910 [details]
Patch
Comment 3 Darin Adler 2016-10-26 18:06:09 PDT
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.
Comment 4 Yusuke Suzuki 2016-10-26 20:37:59 PDT
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 :)
Comment 5 Yusuke Suzuki 2016-10-26 20:45:06 PDT
Committed r207932: <http://trac.webkit.org/changeset/207932>
Comment 6 Yusuke Suzuki 2016-10-27 10:59:24 PDT
jQuery.attr operation is accelerated and shows 15% improvement in Dromaeo jslib-attr-jquery's jQuery.attr(class).
Comment 7 Sam Weinig 2016-10-30 14:59:21 PDT
(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.
Comment 8 Yusuke Suzuki 2016-11-03 15:49:40 PDT
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).
Comment 9 Yusuke Suzuki 2016-11-03 19:43:00 PDT
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&.
Comment 10 Yusuke Suzuki 2016-11-03 22:17:19 PDT
Created attachment 293865 [details]
Patch
Comment 11 Yusuke Suzuki 2016-11-03 22:18:32 PDT
(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...
Comment 12 Yusuke Suzuki 2016-11-03 22:21:19 PDT
Created attachment 293866 [details]
Patch
Comment 13 Yusuke Suzuki 2016-11-04 00:09:02 PDT
Finally, I've reproduced in arm64. Investigating...
Comment 14 Yusuke Suzuki 2016-11-04 10:49:15 PDT
(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.
Comment 15 Yusuke Suzuki 2016-11-04 11:32:07 PDT
(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.
Comment 16 Yusuke Suzuki 2016-11-04 15:07:28 PDT
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
Comment 17 Darin Adler 2016-11-06 10:21:53 PST
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!
Comment 18 Yusuke Suzuki 2016-11-07 11:50:07 PST
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 :)
Comment 19 Yusuke Suzuki 2016-11-09 11:20:05 PST
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).
Comment 20 Yusuke Suzuki 2016-11-09 13:17:34 PST
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.
Comment 21 Yusuke Suzuki 2016-11-09 13:36:31 PST
Committed r208481: <http://trac.webkit.org/changeset/208481>