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

Web Inspector: Crash when inspecting CSS Grid without defined columns or rows #13260

Conversation

patrickangle
Copy link
Contributor

@patrickangle patrickangle commented Apr 28, 2023

05e01b5

Web Inspector: Crash when inspecting CSS Grid without defined columns or rows
https://bugs.webkit.org/show_bug.cgi?id=256072
rdar://108641874

Reviewed by Devin Rousso.

262869@main fixed issues with determining the authored grid track sizes, but in the process introduced a potential null
pointer deref due to us erroneously trying to get a reference to a RefPtr's value instead of getting its pointer for use
in a dynamic downcast.

* LayoutTests/inspector/dom/showFlexOverlay.html:
- Drive-by ensure we enable all options for flex overlays too so that those paths are exercises.

* LayoutTests/inspector/dom/showGridOverlay.html:
* Source/WebCore/inspector/InspectorOverlay.cpp:
(WebCore::authoredGridTrackSizes):

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

2eb0a35

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
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@patrickangle patrickangle self-assigned this Apr 28, 2023
@patrickangle patrickangle added the Web Inspector Bugs related to the WebKit Web Inspector. label Apr 28, 2023
@@ -1441,7 +1441,7 @@ static Vector<String> authoredGridTrackSizes(Node* node, GridTrackSizingDirectio
}
}

auto* cssValueList = dynamicDowncast<CSSValueList>(*cssValue);
auto* cssValueList = dynamicDowncast<CSSValueList>(cssValue.get());
Copy link
Member

Choose a reason for hiding this comment

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

do we need to .get()? or can we just dynamicDowncast<CSSValueList>(cssValue)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we need the .get() here, otherwise we try to downcast the RefPtr itself to CSSValueList, which doesn't compile πŸ˜…

@@ -7,6 +7,14 @@
{
let suite = InspectorTest.createAsyncSuite("DOM.showGridOverlay");

const allEnabledOverlayOptions = {
Copy link
Member

Choose a reason for hiding this comment

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

i wonder if we should do this for the flex overlay too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't hurt.

@patrickangle patrickangle force-pushed the eng/Web-Inspector-Crash-when-inspecting-CSS-Grid-without-defined-columns-or-rows branch from 39e74ad to 2eb0a35 Compare April 28, 2023 15:54
@patrickangle patrickangle added the merge-queue Applied to send a pull request to merge-queue label Apr 28, 2023
… or rows

https://bugs.webkit.org/show_bug.cgi?id=256072
rdar://108641874

Reviewed by Devin Rousso.

262869@main fixed issues with determining the authored grid track sizes, but in the process introduced a potential null
pointer deref due to us erroneously trying to get a reference to a RefPtr's value instead of getting its pointer for use
in a dynamic downcast.

* LayoutTests/inspector/dom/showFlexOverlay.html:
- Drive-by ensure we enable all options for flex overlays too so that those paths are exercises.

* LayoutTests/inspector/dom/showGridOverlay.html:
* Source/WebCore/inspector/InspectorOverlay.cpp:
(WebCore::authoredGridTrackSizes):

Canonical link: https://commits.webkit.org/263517@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Web-Inspector-Crash-when-inspecting-CSS-Grid-without-defined-columns-or-rows branch from 2eb0a35 to 05e01b5 Compare April 28, 2023 21:13
@webkit-commit-queue
Copy link
Collaborator

Committed 263517@main (05e01b5): https://commits.webkit.org/263517@main

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Web Inspector Bugs related to the WebKit Web Inspector.
Projects
None yet
4 participants