Bug 206099 – Null deref crash in DOMWindow::scrollBy after evoking updateLayoutIgnorePendingStylesheets()

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Jack]

<radr://problem/57213791>

Similar to https://webkit.org/b/200409, but the frame is removed by reentrancy after calling updateLayoutIgnorePendingStylesheets.

[Jack]

<rdar://problem/57213791>

[Jack]

Created attachment 387377 [details]
Test case

The test case becomes flaky if we further reduce the html file.

[Jack]

Created attachment 387396 [details]
Patch

[Geoffrey Garen]

Comment on attachment 387396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387396&action=review

> Source/WebCore/page/DOMWindow.cpp:1609
> +    auto frame = makeRefPtr(this->frame());
> +    if (!frame)
> +        return;
>  
> -    FrameView* view = frame()->view();
> +    auto view = makeRefPtr(frame->view());
>      if (!view)
>          return;

In this case, do we even need "frame" and "view" before layout, or should we just delete these accesses, and only use the "afterLayout" variants? (Is there a specific need to perform these null checks? If so, maybe we should just perform them as expressions without saving a local variable at all.)

> Source/WebCore/page/DOMWindow.cpp:1655
> +    // Layout may have affected the current frame:

Can you specify why? For example, is there an event that fires?

[Chris Dumez]

Comment on attachment 387396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387396&action=review

> Source/WebCore/page/DOMWindow.cpp:1604
> +    if (!frame)

This is a bogus check. if (!isCurrentlyDisplayedInFrame()) would have returned early above if the frame was null.

[Simon Fraser (smfr)]

Comment on attachment 387396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387396&action=review

> Source/WebCore/page/DOMWindow.cpp:1644
> +    auto frame = makeRefPtr(this->frame());
> +    if (!frame)
>          return;
>  
> +    auto view = makeRefPtr(frame->view());
> +    if (!view)
> +        return;

We repeat this pattern in various accessors in this file. It's so common I think we should share code for it.

I also think we should limit 'view' and 'frame' to a scope that ends before the updateLayoutIgnorePendingStylesheets call. I see no reason to keep a ref to these across the updateLayoutIgnorePendingStylesheets call.

[Jack]

We had the same discussion. In this test case, frame is null after calling
"updateLayoutIgnorePendingStylesheets". Some have concern about using freed frame pointer, so we use the same approach as in other functions.

(In reply to Geoffrey Garen from comment #4)
> Comment on attachment 387396 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387396&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:1609
> > +    auto frame = makeRefPtr(this->frame());
> > +    if (!frame)
> > +        return;
> >  
> > -    FrameView* view = frame()->view();
> > +    auto view = makeRefPtr(frame->view());
> >      if (!view)
> >          return;
> 
> In this case, do we even need "frame" and "view" before layout, or should we
> just delete these accesses, and only use the "afterLayout" variants? (Is
> there a specific need to perform these null checks? If so, maybe we should
> just perform them as expressions without saving a local variable at all.)
> 
> > Source/WebCore/page/DOMWindow.cpp:1655
> > +    // Layout may have affected the current frame:
> 
> Can you specify why? For example, is there an event that fires?

[Jack]

Thanks! Will either keep this line or add reference count to prevent frame being deleted.

(In reply to Chris Dumez from comment #5)
> Comment on attachment 387396 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387396&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:1604
> > +    if (!frame)
> 
> This is a bogus check. if (!isCurrentlyDisplayedInFrame()) would have
> returned early above if the frame was null.

[Chris Dumez]

(In reply to Jack from comment #8)
> Thanks! Will either keep this line or add reference count to prevent frame
> being deleted.
> 
> (In reply to Chris Dumez from comment #5)
> > Comment on attachment 387396 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=387396&action=review
> > 
> > > Source/WebCore/page/DOMWindow.cpp:1604
> > > +    if (!frame)
> > 
> > This is a bogus check. if (!isCurrentlyDisplayedInFrame()) would have
> > returned early above if the frame was null.

I think Simon already mentioned this but I don't think we want to keep the frame alive past the updateLayoutIgnorePendingStylesheets() call. Instead, you should get the frame *after* the updateLayoutIgnorePendingStylesheets() call and null check it then.

[Jack]

Thanks! Yes, there is a pattern in this file that can be abstracted out.

(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 387396 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387396&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:1644
> > +    auto frame = makeRefPtr(this->frame());
> > +    if (!frame)
> >          return;
> >  
> > +    auto view = makeRefPtr(frame->view());
> > +    if (!view)
> > +        return;
> 
> We repeat this pattern in various accessors in this file. It's so common I
> think we should share code for it.
> 
> I also think we should limit 'view' and 'frame' to a scope that ends before
> the updateLayoutIgnorePendingStylesheets call. I see no reason to keep a ref
> to these across the updateLayoutIgnorePendingStylesheets call.

[Jack]

Geoffrey commented about the lifespan of frame pointer. There was concern about using freed frame pointer so we follow the pattern of other functions.

Will discuss further with Geoffrey and others to decide if we should protect old frame pointer.

(In reply to Chris Dumez from comment #9)
> (In reply to Jack from comment #8)
> > Thanks! Will either keep this line or add reference count to prevent frame
> > being deleted.
> > 
> > (In reply to Chris Dumez from comment #5)
> > > Comment on attachment 387396 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=387396&action=review
> > > 
> > > > Source/WebCore/page/DOMWindow.cpp:1604
> > > > +    if (!frame)
> > > 
> > > This is a bogus check. if (!isCurrentlyDisplayedInFrame()) would have
> > > returned early above if the frame was null.
> 
> I think Simon already mentioned this but I don't think we want to keep the
> frame alive past the updateLayoutIgnorePendingStylesheets() call. Instead,
> you should get the frame *after* the updateLayoutIgnorePendingStylesheets()
> call and null check it then.

[Pinki Gyanchandani]

Created attachment 388966 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 388966 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388966&action=review

r- due to the following issues.

> Source/WebCore/ChangeLog:3
> +        Deploy Ref and RefPtr in DOMWindow::scroll* functions

The title of this bug doesn’t match your patch at all. Please fix it.

> Source/WebCore/ChangeLog:8
> +        Added Null pointer check for frame in scrollBy function before usage.

Nit: null pointer check, not Null pointer check.

> Source/WebCore/page/DOMWindow.cpp:1690
> +    FrameView* view = frame->view();

We should store FrameView in RefPtr. Use auto & makeRefPtr.

> LayoutTests/platform/mac/fast/dom/Window/window-scroll-ignore-null-frame-expected.txt:1
> +layer at (0,0) size 800x600

Please call dumpAsText in the test and add a platform agnostic result.

[Ryosuke Niwa]

Wait a minute, maybe you uploaded your patch to a wrong bug?

[Pinki Gyanchandani]

Created attachment 389062 [details]
Patch

[Pinki Gyanchandani]

Created attachment 389063 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 389063 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389063&action=review

> Source/WebCore/ChangeLog:3
> +        Deploy Ref and RefPtr in DOMWindow::scroll* functions

This line should be updated to reflect the new bug title.

> Source/WebCore/ChangeLog:9
> +        Added null pointer check for frame in scrollBy function before usage.
> +        Test: fast/dom/Window/window-scroll-ignore-null-frame.html

Nit: There should be a blank line between the description & Test: ~ line.

> LayoutTests/ChangeLog:3
> +        Deploy Ref and RefPtr in DOMWindow::scroll* functions

Ditto about updating this line.

> LayoutTests/ChangeLog:8
> +        Addedd null pointer check for frame in scrollBy, before using.

This description should be describing what we're doing in LayoutTests directory.
In this case, you're only adding a test case so it's sufficient to say: Added a regression test.

[Pinki Gyanchandani]

Created attachment 389066 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 389066 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389066&action=review

> LayoutTests/ChangeLog:8
> +        Added null pointer check for frame in scrollBy function before usage.

Again, this line should describe what change we're making in this directory. e.g. "Added a regression test"

> LayoutTests/ChangeLog:10
> +        * fast/dom/Window/window-scroll-ignore-null-frame.html: Added.

This is missing a line for -expected.txt file.

[Pinki Gyanchandani]

Created attachment 389069 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 389069 [details]
Patch

Cool. r=me.

[Ryosuke Niwa]

Let's wait for EWS first. Please also set cq? flag in your patch to ask for your patch to be landed by the commit queue in the future.

[Ryosuke Niwa]

Comment on attachment 389069 [details]
Patch

Looks like iOS EWS is hosed :(

[WebKit Commit Bot]

Comment on attachment 389069 [details]
Patch

Clearing flags on attachment: 389069

Committed r255334: <https://trac.webkit.org/changeset/255334>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.