Skip to content

Commit

Permalink
Fix site isolation overlay crash during teardown
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=274612
rdar://128636304

Reviewed by Charlie Wolfe.

The debug overlays are stored in a WeakHashMap from page to a vector of the overlays.
However, when site isolation is on, whenever a value is being removed from the WeakHashMap
because the page has been destrucuted, the overlay will try to access a WeakRef to the page,
which crashes. To fix this, we make the page in a RegionOverlay stored as WeakPtr instead and
check it before accessing it.

* Source/WebCore/page/DebugPageOverlays.cpp:
(WebCore::MouseWheelRegionOverlay::updateRegion):
(WebCore::NonFastScrollableRegionOverlay::updateRegion):
(WebCore::InteractionRegionOverlay::activeLayer const):
(WebCore::InteractionRegionOverlay::activeRegion const):
(WebCore::InteractionRegionOverlay::mouseEvent):
(WebCore::SiteIsolationOverlay::drawRect):
(WebCore::RegionOverlay::~RegionOverlay):
(WebCore::RegionOverlay::protectedPage const): Deleted.

Canonical link: https://commits.webkit.org/279386@main
  • Loading branch information
Pascoe committed May 28, 2024
1 parent c076205 commit 4d646a4
Showing 1 changed file with 29 additions and 9 deletions.
38 changes: 29 additions & 9 deletions Source/WebCore/page/DebugPageOverlays.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,8 @@ class RegionOverlay : public RefCounted<RegionOverlay>, public PageOverlayClient
// Returns true if the region changed.
virtual bool updateRegion() = 0;
void drawRegion(GraphicsContext&, const Region&, const Color&, const IntRect& dirtyRect);
Ref<Page> protectedPage() const { return m_page.get(); }

SingleThreadWeakRef<Page> m_page;
SingleThreadWeakPtr<Page> m_page;
RefPtr<PageOverlay> m_overlay;
std::unique_ptr<Region> m_region;
Color m_color;
Expand Down Expand Up @@ -111,13 +110,16 @@ class MouseWheelRegionOverlay final : public RegionOverlay {

bool MouseWheelRegionOverlay::updateRegion()
{
RefPtr page = m_page.get();
if (!page)
return false;
#if ENABLE(WHEEL_EVENT_REGIONS)
// Wheel event regions are painted via RenderLayerBacking::paintDebugOverlays().
return false;
#else
auto region = makeUnique<Region>();

for (auto* frame = &m_page->mainFrame(); frame; frame = frame->tree().traverseNext()) {
for (RefPtr frame = &page->mainFrame(); frame; frame = frame->tree().traverseNext()) {
RefPtr localFrame = dynamicDowncast<LocalFrame>(frame);
if (!localFrame)
continue;
Expand Down Expand Up @@ -163,6 +165,9 @@ class NonFastScrollableRegionOverlay final : public RegionOverlay {

bool NonFastScrollableRegionOverlay::updateRegion()
{
RefPtr page = m_page.get();
if (!page)
return false;
bool regionChanged = false;

if (RefPtr scrollingCoordinator = m_page->scrollingCoordinator()) {
Expand Down Expand Up @@ -314,13 +319,16 @@ static Vector<Path> pathsForRect(const FloatRect& rect, float borderRadius)

std::optional<std::pair<RenderLayer&, GraphicsLayer&>> InteractionRegionOverlay::activeLayer() const
{
RefPtr page = m_page.get();
if (!page)
return std::nullopt;
constexpr OptionSet<HitTestRequest::Type> hitType {
HitTestRequest::Type::ReadOnly,
HitTestRequest::Type::Active,
HitTestRequest::Type::AllowChildFrameContent
};
HitTestResult result(m_mouseLocationInContentCoordinates);
RefPtr localMainFrame = dynamicDowncast<LocalFrame>(m_page->mainFrame());
RefPtr localMainFrame = dynamicDowncast<LocalFrame>(page->mainFrame());
if (!localMainFrame)
return std::nullopt;

Expand Down Expand Up @@ -352,6 +360,9 @@ std::optional<std::pair<RenderLayer&, GraphicsLayer&>> InteractionRegionOverlay:
std::optional<InteractionRegion> InteractionRegionOverlay::activeRegion() const
{
#if ENABLE(INTERACTION_REGIONS_IN_EVENT_REGION)
RefPtr page = m_page.get();
if (!page)
return std::nullopt;
auto layerPair = activeLayer();
if (!layerPair)
return std::nullopt;
Expand All @@ -361,7 +372,7 @@ std::optional<InteractionRegion> InteractionRegionOverlay::activeRegion() const
IntRect hitRectInOverlayCoordinates;
float hitRegionArea = 0;

auto* localMainFrame = dynamicDowncast<LocalFrame>(m_page->mainFrame());
auto* localMainFrame = dynamicDowncast<LocalFrame>(page->mainFrame());
if (!localMainFrame)
return std::nullopt;

Expand Down Expand Up @@ -596,7 +607,10 @@ void InteractionRegionOverlay::drawRect(PageOverlay&, GraphicsContext& context,

bool InteractionRegionOverlay::mouseEvent(PageOverlay& overlay, const PlatformMouseEvent& event)
{
RefPtr localMainFrame = dynamicDowncast<LocalFrame>(m_page->mainFrame());
RefPtr page = m_page.get();
if (!page)
return false;
RefPtr localMainFrame = dynamicDowncast<LocalFrame>(page->mainFrame());
if (!localMainFrame)
return false;
RefPtr mainFrameView = localMainFrame->view();
Expand All @@ -615,7 +629,7 @@ bool InteractionRegionOverlay::mouseEvent(PageOverlay& overlay, const PlatformMo
cursorToSet = handCursor();
if (event.button() == MouseButton::Left && event.type() == PlatformEvent::Type::MousePressed) {
m_settings[i].value = !m_settings[i].value;
protectedPage()->forceRepaintAllFrames();
page->forceRepaintAllFrames();
return true;
}
}
Expand Down Expand Up @@ -663,6 +677,9 @@ bool SiteIsolationOverlay::updateRegion()

void SiteIsolationOverlay::drawRect(PageOverlay&, GraphicsContext& context, const IntRect&)
{
RefPtr page = m_page.get();
if (!page)
return;
GraphicsContextStateSaver stateSaver(context);

FontCascadeDescription fontDescription;
Expand All @@ -673,7 +690,7 @@ void SiteIsolationOverlay::drawRect(PageOverlay&, GraphicsContext& context, cons
FontCascade font(WTFMove(fontDescription));
font.update(nullptr);

for (auto* frame = &m_page->mainFrame(); frame; frame = frame->tree().traverseNext()) {
for (RefPtr frame = &page->mainFrame(); frame; frame = frame->tree().traverseNext()) {
if (!frame->virtualView())
continue;
auto frameView = frame->virtualView();
Expand Down Expand Up @@ -719,8 +736,11 @@ RegionOverlay::RegionOverlay(Page& page, Color regionColor)

RegionOverlay::~RegionOverlay()
{
RefPtr page = m_page.get();
if (!page)
return;
if (RefPtr overlay = m_overlay)
protectedPage()->pageOverlayController().uninstallPageOverlay(*overlay, PageOverlay::FadeMode::DoNotFade);
page->pageOverlayController().uninstallPageOverlay(*overlay, PageOverlay::FadeMode::DoNotFade);
}

void RegionOverlay::willMoveToPage(PageOverlay&, Page* page)
Expand Down

0 comments on commit 4d646a4

Please sign in to comment.