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().
<rdar://72894454>
(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.
Created attachment 418791 [details] patch
Can we reduce the test? It seems like most of JS & CSS shouldn't be necessary.
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 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 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.
Created attachment 418911 [details] patch for landing
Committed r272174: <https://trac.webkit.org/changeset/272174> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418911 [details].