Bug 164004 - [DOMJIT] Implement Node::ownerDocument
Summary: [DOMJIT] Implement Node::ownerDocument
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on: 164432
Blocks: 162544 164006
  Show dependency treegraph
 
Reported: 2016-10-25 22:03 PDT by Yusuke Suzuki
Modified: 2016-11-09 13:36 PST (History)
11 users (show)

See Also:


Attachments
Patch (11.52 KB, patch)
2016-10-26 02:35 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (10.66 KB, patch)
2016-11-03 22:17 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (10.67 KB, patch)
2016-11-03 22:21 PDT, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>