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.

Sign up for GitHub

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

Jump to bottom

XSLImportRule and XSLStyleSheet should use weak pointers instead of raw pointers #3522

Merged
merged 1 commit into from Aug 23, 2022
Merged

XSLImportRule and XSLStyleSheet should use weak pointers instead of raw pointers #3522

merged 1 commit into from Aug 23, 2022

Conversation

rniwa
Copy link
Member

@rniwa rniwa commented Aug 22, 2022 •
edited by webkit-early-warning-system

896fddc

XSLImportRule and XSLStyleSheet should use weak pointers instead of raw pointers
https://bugs.webkit.org/show_bug.cgi?id=244181

Reviewed by Darin Adler.

Deploy WeakPtr where raw pointers are used.

* Source/WebCore/dom/ProcessingInstruction.cpp:
(WebCore::ProcessingInstruction::setXSLStyleSheet):
* Source/WebCore/xml/XSLImportRule.cpp:
(WebCore::XSLImportRule::XSLImportRule):
(WebCore::XSLImportRule::setXSLStyleSheet):
(WebCore::XSLImportRule::loadSheet):
* Source/WebCore/xml/XSLImportRule.h:
(WebCore::XSLImportRule::parentStyleSheet const):
* Source/WebCore/xml/XSLStyleSheet.h:
* Source/WebCore/xml/XSLStyleSheetLibxslt.cpp:
(WebCore::XSLStyleSheet::XSLStyleSheet):
(WebCore::XSLStyleSheet::checkLoaded):
(WebCore::XSLStyleSheet::loadChildSheet):
(WebCore::XSLStyleSheet::ownerDocument):
(WebCore::XSLStyleSheet::locateStylesheetSubResource):

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

57d9247

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 api-mac ❌ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk1
✅ 🛠 tv-sim ✅ 🧪 mac-wk2
✅ 🛠 watch ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress

@rniwa rniwa requested a review from cdumez as a code owner August 22, 2022 03:50
@rniwa rniwa self-assigned this Aug 22, 2022
@rniwa rniwa added DOM For bugs specific to XML/HTML DOM elements (including parsing). WebKit Nightly Build labels Aug 22, 2022
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 1b8d891)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ❌ 🛠 ios ⏳ 🛠 mac ❌ 🛠 wpe   🛠 🧪 win
✅ 🧪 bindings ❌ 🛠 ios-sim ⏳ 🛠 mac-debug   🛠 gtk   🛠 wincairo
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ⏳ 🛠 mac-AS-debug   🧪 gtk-wk2
❌ 🧪 api-ios ⏳ 🧪 api-mac   🧪 api-gtk
⏳ 🛠 🧪 jsc ❌ 🛠 tv ⏳ 🧪 mac-wk1
❌ 🛠 tv-sim ⏳ 🧪 mac-wk2
❌ 🛠 watch ⏳ 🧪 mac-AS-debug-wk2
❌ 🛠 watch-sim ⏳ 🧪 mac-wk2-stress

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 22, 2022
@rniwa rniwa removed the merging-blocked Applied to prevent a change from being merged label Aug 22, 2022
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 4502008)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 api-mac ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk1
✅ 🛠 tv-sim ✅ 🧪 mac-wk2
✅ 🛠 watch ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress

@rniwa rniwa requested review from anttijk, ddkilzer, brentfulgham and darinadler and removed request for ddkilzer and brentfulgham August 22, 2022 18:36
darinadler
darinadler approved these changes Aug 23, 2022
View reviewed changes
Source/WebCore/xml/XSLImportRule.cpp
XSLStyleSheet* parent = parentStyleSheet();
if (parent)
m_styleSheet->setParentStyleSheet(parent);
// FIXME: parentStyleSheet() should never be null here.
Copy link
Member

Choose a reason for hiding this comment

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

I don’t understand why this comment says FIXME. What are we going to fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, we pass a reference instead of a pointer if it's never null. I can't easily validate that, however.

Source/WebCore/xml/XSLImportRule.cpp
rootSheet = parentSheet;
}

if (rootSheet)
cachedResourceLoader = rootSheet->cachedResourceLoader();
RefPtr cachedResourceLoader = rootSheet->cachedResourceLoader();
Copy link
Member

Choose a reason for hiding this comment

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

The code just above this checks rootSheet for null, but here we dereference the pointer unconditionally. The old code didn’t do that.

Copy link
Member Author

@rniwa rniwa Aug 23, 2022 •
edited

Choose a reason for hiding this comment

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

Yes, this is true but line 99/106 below also unconditionally dereferences the resource loader.

Whilst it's after an early exit for a cyclic relationship, I don't think that's relevant as far as nullify of the resource loader is concerned.

Copy link
Member

Choose a reason for hiding this comment

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

We could remove the unnecessary null check above then?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose we could. I'll go do that.

@webkit-early-warning-system
Copy link
Collaborator

57d9247

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 api-mac ❌ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk1
✅ 🛠 tv-sim ✅ 🧪 mac-wk2
✅ 🛠 watch ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress

@rniwa rniwa added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Aug 23, 2022
@rniwa
XSLImportRule and XSLStyleSheet should use weak pointers instead of r…
896fddc 
…aw pointers

https://bugs.webkit.org/show_bug.cgi?id=244181

Reviewed by Darin Adler.

Deploy WeakPtr where raw pointers are used.

* Source/WebCore/dom/ProcessingInstruction.cpp:
(WebCore::ProcessingInstruction::setXSLStyleSheet):
* Source/WebCore/xml/XSLImportRule.cpp:
(WebCore::XSLImportRule::XSLImportRule):
(WebCore::XSLImportRule::setXSLStyleSheet):
(WebCore::XSLImportRule::loadSheet):
* Source/WebCore/xml/XSLImportRule.h:
(WebCore::XSLImportRule::parentStyleSheet const):
* Source/WebCore/xml/XSLStyleSheet.h:
* Source/WebCore/xml/XSLStyleSheetLibxslt.cpp:
(WebCore::XSLStyleSheet::XSLStyleSheet):
(WebCore::XSLStyleSheet::checkLoaded):
(WebCore::XSLStyleSheet::loadChildSheet):
(WebCore::XSLStyleSheet::ownerDocument):
(WebCore::XSLStyleSheet::locateStylesheetSubResource):

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

Committed 253695@main (896fddc): https://commits.webkit.org/253695@main

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

@webkit-early-warning-system webkit-early-warning-system merged commit 896fddc into WebKit:main Aug 23, 2022
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Aug 23, 2022
@rniwa rniwa deleted the fix244181 branch August 23, 2022 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darinadler darinadler darinadler approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@anttijk anttijk Awaiting requested review from anttijk

Assignees

@rniwa rniwa

Labels
DOM For bugs specific to XML/HTML DOM elements (including parsing).
Projects
None yet
Milestone
No milestone
5 participants
@rniwa @webkit-early-warning-system @webkit-commit-queue @darinadler @webkit-ews-buildbot