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

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Saam Barati]

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().

[Saam Barati]

<rdar://72894454>

[Saam Barati]

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

[Saam Barati]

Created attachment 418791 [details]
patch

[Ryosuke Niwa]

Can we reduce the test? It seems like most of JS & CSS shouldn't be necessary.

[Ryosuke Niwa]

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?

[Mark Lam]

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

[Saam Barati]

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.

[Saam Barati]

Created attachment 418911 [details]
patch for landing

[EWS]

Committed r272174: <https://trac.webkit.org/changeset/272174>

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