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.
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.
Created attachment 292910 [details] Patch
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 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 :)
Committed r207932: <http://trac.webkit.org/changeset/207932>
jQuery.attr operation is accelerated and shows 15% improvement in Dromaeo jslib-attr-jquery's jQuery.attr(class).
(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.
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).
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&.
Created attachment 293865 [details] Patch
(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...
Created attachment 293866 [details] Patch
Finally, I've reproduced in arm64. Investigating...
(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.
(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.
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 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 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 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 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.
Committed r208481: <http://trac.webkit.org/changeset/208481>