Bug 221098 - Lazily create m_windowCloseWatchpoints so we don't mistakenly think we have a frame when re-associating a document to a given cached frame
Summary: Lazily create m_windowCloseWatchpoints so we don't mistakenly think we have a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-28 13:35 PST by Saam Barati
Modified: 2021-02-01 13:41 PST (History)
11 users (show)

See Also:


Attachments
patch (105.54 KB, patch)
2021-01-29 17:18 PST, Saam Barati
rniwa: review+
Details | Formatted Diff | Diff
patch for landing (105.51 KB, patch)
2021-02-01 12:16 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2021-01-28 13:35:41 PST
We only fire this watchpoint set once, but a page may repeatedly open/close w.r.t back forward cache. This breaks invariants on debug builds, leading to debug crashes, since we try to watch an already fired watchpoint.

On a release build, I'm guessing this could lead to us being able to access properties on N+1 closes that we shouldn't access otherwise, since the watchpoint won't fire on N+1.

I think the solution could be as simple as just creating new watchpoint sets each time we need it, and null-ing it out on frameDestroyed().
Comment 1 Saam Barati 2021-01-29 16:58:34 PST
<rdar://72894454>
Comment 2 Saam Barati 2021-01-29 17:02:24 PST
(In reply to Saam Barati from comment #0)
> We only fire this watchpoint set once, but a page may repeatedly open/close
> w.r.t back forward cache. This breaks invariants on debug builds, leading to
> debug crashes, since we try to watch an already fired watchpoint.
> 
> On a release build, I'm guessing this could lead to us being able to access
> properties on N+1 closes that we shouldn't access otherwise, since the
> watchpoint won't fire on N+1.
> 
> I think the solution could be as simple as just creating new watchpoint sets
> each time we need it, and null-ing it out on frameDestroyed().

This wasn't what was happening.

There's a scenario where we go back when using the back forward cache, and we reassociate a cached frame with a document that we end up in this state where we create a JSDOMWindow that thinks it doesn't have a frame, even though it does. It's just because we haven't finished the bookkeeping in `FrameLoader::open(CachedFrameBase& cachedFrame)` that JSDOMWindow doesn't think it has a frame. If we wait to create this watchpoint lazily, when it's actually needed, the JSDOMWindow will be in a state where we can evaluate if it has a frame or not.
Comment 3 Saam Barati 2021-01-29 17:18:55 PST
Created attachment 418791 [details]
patch
Comment 4 Ryosuke Niwa 2021-01-29 17:28:11 PST
Can we reduce the test? It seems like most of JS & CSS shouldn't be necessary.
Comment 5 Ryosuke Niwa 2021-01-29 18:05:01 PST
Comment on attachment 418791 [details]
patch

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

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:250
> +    // Because this can only happen once, we don't need to consider a potential ping-ponging
> +    // between states of having a frame or not having a frame.

This is a bit misleading. It's possible for DOMWindow::frame() to temporarily return nullptr while it's inside a page cache.
Maybe rephrase it as once the frame is *destroyed* and that DOMWindow can be associated with at most one frame in its lifetime?
Comment 6 Mark Lam 2021-01-29 18:08:57 PST
Comment on attachment 418791 [details]
patch

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

r=me too

> Source/WebCore/ChangeLog:14
> +        even though we did. It was because we haden't finished the bookkeeping in

/haden't/hadn't/

> Source/WebCore/ChangeLog:16
> +        frame. And DOMWindow  relies on its document to get its frame. 

Extra space after `DOMWindow`.
Comment 7 Saam Barati 2021-02-01 12:10:22 PST
Comment on attachment 418791 [details]
patch

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

>> Source/WebCore/ChangeLog:14
>> +        even though we did. It was because we haden't finished the bookkeeping in
> 
> /haden't/hadn't/

will fix

>> Source/WebCore/ChangeLog:16
>> +        frame. And DOMWindow  relies on its document to get its frame. 
> 
> Extra space after `DOMWindow`.

will fix

>> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:250
>> +    // between states of having a frame or not having a frame.
> 
> This is a bit misleading. It's possible for DOMWindow::frame() to temporarily return nullptr while it's inside a page cache.
> Maybe rephrase it as once the frame is *destroyed* and that DOMWindow can be associated with at most one frame in its lifetime?

Will rephrase like you suggest.
Comment 8 Saam Barati 2021-02-01 12:16:38 PST
Created attachment 418911 [details]
patch for landing
Comment 9 EWS 2021-02-01 13:41:01 PST
Committed r272174: <https://trac.webkit.org/changeset/272174>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418911 [details].