Bug 221098

Summary: Lazily create m_windowCloseWatchpoints so we don't mistakenly think we have a frame when re-associating a document to a given cached frame
Product: WebKit Reporter: Saam Barati <saam>
Component: WebCore JavaScriptAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, ews-watchlist, fpizlo, ggaren, keith_miller, mark.lam, msaboff, rniwa, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
rniwa: review+
patch for landing none

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].