Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move data members specific to ElementRareData out of NodeRareData #11131

Merged

Conversation

cdumez
Copy link
Contributor

@cdumez cdumez commented Mar 6, 2023

11ece51

Move data members specific to ElementRareData out of NodeRareData
https://bugs.webkit.org/show_bug.cgi?id=253461

Reviewed by Darin Adler.

Move data members specific to ElementRareData out of NodeRareData. Those were
put in NodeRareData for better packing but we can achieve the same packing by
reordering data members.

I have verified that sizeof(NodeRareData)=32 and sizeof(ElementRareDate)=232
before and after my change.

* Source/WebCore/dom/ElementRareData.cpp:
* Source/WebCore/dom/ElementRareData.h:
(WebCore::ElementRareData::useTypes const):
* Source/WebCore/dom/NodeRareData.cpp:
* Source/WebCore/dom/NodeRareData.h:
(WebCore::NodeRareData::isElementRareData const):
(WebCore::NodeRareData::useTypes const):
(WebCore::NodeRareData::isElementRareData): Deleted.

Canonical link: https://commits.webkit.org/261308@main

a40448e

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch   πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch-sim

@cdumez cdumez requested a review from rniwa as a code owner March 6, 2023 21:58
@cdumez cdumez self-assigned this Mar 6, 2023
@cdumez cdumez added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label Mar 6, 2023
@@ -263,7 +263,7 @@ class NodeRareData {
{
}

bool isElementRareData() { return m_isElementRareData; }
bool isElementRareData() const { return m_isElementRareData; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? Don't we already know if it's an Element because of a flag in Node?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's true that Node has a NodeFlag that indicates whether the Node is an Element or not. I think you're right that we could probably infer wether the the NodeRareData is an ElementRareData. I'll look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we use this flag in NodeRareData's operator delete() to make sure we delete ElementRareData data members too, since we don't use virtual inheritance (likely as an optimization).

Node knows whether it is NodeRareData or an ElementRareData. However, NodeRareData doesn't have a reference to its Node and doesn't know if it is an ElementRareData without this flag.

One way I guess would be to have Node to manual memory management and explicitly cast to the subclass before calling delete. However, this would be more error prone.

Copy link
Member

@rniwa rniwa Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's precisely why we have that boolean. The type information needs to be stored in the object whose type is determined by it. Otherwise, the type & type info can get out of sync and may lead to an exploitable type confusion bug.

@cdumez cdumez added the merge-queue Applied to send a pull request to merge-queue label Mar 7, 2023
https://bugs.webkit.org/show_bug.cgi?id=253461

Reviewed by Darin Adler.

Move data members specific to ElementRareData out of NodeRareData. Those were
put in NodeRareData for better packing but we can achieve the same packing by
reordering data members.

I have verified that sizeof(NodeRareData)=32 and sizeof(ElementRareDate)=232
before and after my change.

* Source/WebCore/dom/ElementRareData.cpp:
* Source/WebCore/dom/ElementRareData.h:
(WebCore::ElementRareData::useTypes const):
* Source/WebCore/dom/NodeRareData.cpp:
* Source/WebCore/dom/NodeRareData.h:
(WebCore::NodeRareData::isElementRareData const):
(WebCore::NodeRareData::useTypes const):
(WebCore::NodeRareData::isElementRareData): Deleted.

Canonical link: https://commits.webkit.org/261308@main
@webkit-commit-queue
Copy link
Collaborator

Committed 261308@main (11ece51): https://commits.webkit.org/261308@main

Reviewed commits have been landed. Closing PR #11131 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 11ece51 into WebKit:main Mar 7, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
5 participants