Bug 34679 – Location and other objects are dysfunctional after a document gets restored from b/f cache

• Component: Page Loading
• Status: RESOLVED FIXED

["Cowboy" Ben Alman]

This is somewhat hard to describe... but at least it's reproducible:
http://benalman.com/code/projects/jquery-hashchange/examples/bug-safari-back-from-diff-domain/

The gist is, when you navigate to a different domain, and press the back button, accessing 'loc.href' from reference 'var loc = window.location' is sometimes broken. I don't know if this is a "location" thing or a general "reference" thing, but it's totally broken here.

I have confirmed this behavior on machines other than mine, and in both Safari 4.0.4 and WebKit nightly r54415.

The following is explained at the example page, but I will include it here for posterity.

This is the problematic code:

(function(window){
  
  // A convenient shortcut.
  var win_loc = window.location,
    loc = location,
    state;
  
  (function loopy(){
    console.log([
      'win_loc.href: ' + typeof win_loc.href,
      'loc.href: ' + typeof loc.href,
      'location.href: ' + typeof location.href,
      'window.location.href: ' + typeof window.location.href
    ].join(', '));
    timeout_id = setTimeout( loopy, 1000 );
  })();
  
})(this);

When you initially navigate to the page, this is logged:
win_loc.href: string, loc.href: string, location.href: string, window.location.href: string

After visiting another domain, and pressing the back button, this is logged:
win_loc.href: undefined, loc.href: undefined, location.href: string, window.location.href: string

So, there you go.

[Alexey Proskuryakov]

We call Location::disconnectFrame() when a document goes to b/f cache, but we never reconnect the frame when it's restored.

This is an issue for many objects - one can easily find a list in DOMWindow::clear(), since that's where we call their disconnectFrame() methods.

[Sam Weinig]

<rdar://problem/7908830>

[Alexey Proskuryakov]

*** Bug 54960 has been marked as a duplicate of this bug. ***

[Adam Barth]

Yep.  I don't know if we'll be able to solve this problem.

[Rob B]

Created attachment 125471 [details]
Example of problem with iframes and iOS 5 b/f cache

[Rob B]

We've been hitting this bug pretty hard in mobile Safari since iOS 5 came out. On iOS 4, we could get around this by preventing b/f cache by adding an empty window 'unload' handler, but in iOS 5 Safari this doesn't seem to prevent a page from loading from b/f cache.

Basically, load up the attached index.html (which loads inner.html as an iframe) in iOS 5 Safari. On first load / reload of index.html, you can click send ping to iframe and it will correctly use postMessage to increment the ping's received count inside the iframe. When you click back then fwd and this page loads from the b/f cache in iOS 5, a few bugs occur: 

(a) the iframe listeners are disconnected so pings don't get received by iframe 
(b) anything dynamic in the iframe doesn't work solely inside the iframe itself

This is a huge issue for mobile advertising or anywhere that uses iframes (or any somewhat complex JS). At the very least, is there a known way in iOS 5 / later Webkit versions to prevent a page from loading in b/f cache to work around this bug? (Again, note an empty window unload handler no longer works as workaround in iOS 5 Safari.)

It would be awesome to have iframes, event listeners, JS, etc. work in the b/f cache.

[Henrik Tengelin]

It also seems like event handlers added to window object is lost when returning to page using back button. 
I have tested with scroll, resize, error, all triggered normally when first navigating to page, but none is triggered after navigating to other page and then returning using back button.

There are also numerous reports from iOS 5 users getting an "older version" of the page when using back button. Haven't confirmed it, but i suspect page cache to be involved here as well.

[Brady Eidson]

(In reply to comment #7)
> It also seems like event handlers added to window object is lost when returning to page using back button. 
> I have tested with scroll, resize, error, all triggered normally when first navigating to page, but none is triggered after navigating to other page and then returning using back button.

That's all this same bug.

I'm working on this right now, btw (Just assigned the bug to myself)
 
> There are also numerous reports from iOS 5 users getting an "older version" of the page when using back button. Haven't confirmed it, but i suspect page cache to be involved here as well.

Nope, not at all the same issue.

[Brady Eidson]

The recent addition of DOMWindowProperty is quite relevant to fixing this bug.  All of the things we want to save and restore are DOMWindowProperties, and all of the things that might prevent page-cacheability are already also ActiveDOMProperties.

The first problem that comes up is that we need to keep the objects we wish to save/restore alive in the page cache.  Right now all of these objects are RefCounted, but with their own base class as the template specialization.

My first task as a refactor will be to flip that around so DOMWindowProperty is RefCounted and all the classes derive from that.

[Adam Barth]

> The recent addition of DOMWindowProperty is quite relevant to fixing this bug.  All of the things we want to save and restore are DOMWindowProperties, and all of the things that might prevent page-cacheability are already also ActiveDOMProperties.

Correct.  I've been trying to work towards fixing this bug for a while.  :)

The idea I had in mind was to create a "reconnectFrame" method to parallel disconnectFrame.

> The first problem that comes up is that we need to keep the objects we wish to save/restore alive in the page cache.  Right now all of these objects are RefCounted, but with their own base class as the template specialization.
> 
> My first task as a refactor will be to flip that around so DOMWindowProperty is RefCounted and all the classes derive from that.

Ok.  I think you can solve this problem without going that route, but that's a fine route to go as well.  Feel free to grab me on #webkit if you'd like to discuss further.

[Brady Eidson]

(In reply to comment #10)
> > The recent addition of DOMWindowProperty is quite relevant to fixing this bug.  All of the things we want to save and restore are DOMWindowProperties, and all of the things that might prevent page-cacheability are already also ActiveDOMProperties.
> 
> Correct.  I've been trying to work towards fixing this bug for a while.  :)
> 
> The idea I had in mind was to create a "reconnectFrame" method to parallel disconnectFrame.

I already have that implemented and stashed away as a proof of concept.  Exact same name.  ;)
> 
> > The first problem that comes up is that we need to keep the objects we wish to save/restore alive in the page cache.  Right now all of these objects are RefCounted, but with their own base class as the template specialization.
> > 
> > My first task as a refactor will be to flip that around so DOMWindowProperty is RefCounted and all the classes derive from that.
> 
> Ok.  I think you can solve this problem without going that route, but that's a fine route to go as well.  Feel free to grab me on #webkit if you'd like to discuss further.

I *could* solve it as-is by having the page cache remember a whole bunch of explicit objects like "Location" and "History" and "Navigator" and "StyleMedia", etc.

But I'd really like to get away from the little bit of hell that exists in DOMWindow right now where each of them has to be called out explicitly with a special type of RefPtr.

Unfortunately It turns out I was wrong about the at-a-glance assumption that all DOMWindowProperties were already RefCounted - DOMWindowIndexedDatabase.h and NavigatorGeolocation.h are the exceptions.

Seems like I have two directions I could go:
1 - Refactor those two classes into being RefCounted, then proceed with my original refactor.
2 - Do something else that you might already have in mind.

Thoughts?

[Adam Barth]

> I *could* solve it as-is by having the page cache remember a whole bunch of explicit objects like "Location" and "History" and "Navigator" and "StyleMedia", etc.

That seems undesirable.

> But I'd really like to get away from the little bit of hell that exists in DOMWindow right now where each of them has to be called out explicitly with a special type of RefPtr.

Why does the PageCache need to know any of this?  We get a new DOMWindow object for each Document, so the DOMWindow can be responsible for dealing with its properties.  The PageCache just need to signal the DOMWindow that it's time to reconnect everybody.

> Unfortunately It turns out I was wrong about the at-a-glance assumption that all DOMWindowProperties were already RefCounted - DOMWindowIndexedDatabase.h and NavigatorGeolocation.h are the exceptions.

I don't think either of these need to be DOMWindowProperties anymore.  They can just be FrameDestructionObservers.  Notice: NavigatorGeolocation doesn't override disconnectFrame anymore and DOMWindowIndexedDatabase just does so to clear out m_idbFactory.

> Seems like I have two directions I could go:
> 1 - Refactor those two classes into being RefCounted, then proceed with my original refactor.
> 2 - Do something else that you might already have in mind.

The approach I had in mind was the following:

1) Add the reconnectFrame method to DOMWindowProperty and implement whatever is necessary in each subclass to actually reconnect to the frame.

2) For each property getter (e.g., DOMWindow::screen), change it to always return 0 when isCurrentlyDisplayedInFrame() is false.

3) In DOMWindow::clear(), stop zeroing out the various RefPtrs in DOMWindow.

I think that's all that's needed to fix this bug at this point, but I might be missing something in the details.

[Brady Eidson]

(In reply to comment #12)
> > But I'd really like to get away from the little bit of hell that exists in DOMWindow right now where each of them has to be called out explicitly with a special type of RefPtr.
> 
> Why does the PageCache need to know any of this?  We get a new DOMWindow object for each Document, so the DOMWindow can be responsible for dealing with its properties.  The PageCache just need to signal the DOMWindow that it's time to reconnect everybody.

I misunderstood what role DOMWindow played in the split window model: I thought DOMWindow was the object that was reused.

Your response here made me go look much closer and realize that it's not and that I've been leading myself on a wild goose chase.

> The approach I had in mind was the following:
> 
> 1) Add the reconnectFrame method to DOMWindowProperty and implement whatever is necessary in each subclass to actually reconnect to the frame.
> 
> 2) For each property getter (e.g., DOMWindow::screen), change it to always return 0 when isCurrentlyDisplayedInFrame() is false.
> 
> 3) In DOMWindow::clear(), stop zeroing out the various RefPtrs in DOMWindow.
> 
> I think that's all that's needed to fix this bug at this point, but I might be missing something in the details.

With the realization that DOMWindows aren't reused (my bad) this is quite obviously the way to go.  Will plow through that tomorrow.

[Brady Eidson]

Working on this right now and discovered...

(In reply to comment #13)
> (In reply to comment #12)
> > The approach I had in mind was the following:
> > ...
> > 2) For each property getter (e.g., DOMWindow::screen), change it to always return 0 when isCurrentlyDisplayedInFrame() is false.
> > 

They all already do the right thing.  huzzah!

[Adam Barth]

> They all already do the right thing.  huzzah!

DOMWindow::screen doesn't, for example:

http://trac.webkit.org/browser/trunk/Source/WebCore/page/DOMWindow.cpp#L552

If m_screen is already non-0, then it will return the object.  Otherwise, it will return 0.  That seems undesirable because it leaks the fact that we're creating these objects lazily.

[Brady Eidson]

(In reply to comment #15)
> > They all already do the right thing.  huzzah!
> 
> DOMWindow::screen doesn't, for example:
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/page/DOMWindow.cpp#L552
> 
> If m_screen is already non-0, then it will return the object.  Otherwise, it will return 0.  That seems undesirable because it leaks the fact that we're creating these objects lazily.

This code?

Screen* DOMWindow::screen() const
{
    if (!m_screen && isCurrentlyDisplayedInFrame())
        m_screen = Screen::create(m_frame);
    return m_screen.get();
}

"If m_screen is NULL *and* this DOMWindow is currently displayed in frame, then lazily create m_screen"

If the DOMWindow is in the page cache, isCurrentlyDisplayedInFrame() will be false, and m_screen's 0-pointer will be returned.

[Brady Eidson]

(In reply to comment #16)
> (In reply to comment #15)
> > > They all already do the right thing.  huzzah!
> > 
> > DOMWindow::screen doesn't, for example:
> > 
> > http://trac.webkit.org/browser/trunk/Source/WebCore/page/DOMWindow.cpp#L552
> > 
> > If m_screen is already non-0, then it will return the object.  Otherwise, it will return 0.  That seems undesirable because it leaks the fact that we're creating these objects lazily.
> 
> This code?
> 
> Screen* DOMWindow::screen() const
> {
>     if (!m_screen && isCurrentlyDisplayedInFrame())
>         m_screen = Screen::create(m_frame);
>     return m_screen.get();
> }
> 
> "If m_screen is NULL *and* this DOMWindow is currently displayed in frame, then lazily create m_screen"
> 
> If the DOMWindow is in the page cache, isCurrentlyDisplayedInFrame() will be false, and m_screen's 0-pointer will be returned.

Ignore that!

I was completely missing the point.

I'm on board now.  :)

[Adam Barth]

> 3) In DOMWindow::clear(), stop zeroing out the various RefPtrs in DOMWindow.

Yeah, step (3) is important because you want window.location === window.location both before and after the page has been in the PageCache.  (While the page is in the page cache, it's ok for window.location be null---that's a bug for another day.)

[Brady Eidson]

Finished up a patch, wrote a thorough layout test, and discovered some crashes when restoring Local/SessionStorage objects.

The "Storage" object is a wrapper around a StorageArea.
DOMWindow creates its own Storage object wrapping its LocalStorage area, which gets registered as one of its DOMWindowProperties.
The WebInspector also creates its own Storage object wrapping the same LocalStorage area which also gets registered as one of the DOMWindow's DOMWindowProperties.

Then you leave the page and it is cached, and we disconnect all DOMWindowProperties from the frame.

Then the WebInspector's wrapper is cleaned up.  In the ::~Storage destructor it can't unregister itself from the DOMWindow since it doesn't have its Frame* any long.

Then you go back, and the DOMWindow tries to reconnect this invalid Storage object.

I'm working on coming up with a clever solution to this without bloating the DOMWindowProperty mechanism that much.

[Brady Eidson]

(In reply to comment #19)
> Finished up a patch, wrote a thorough layout test, and discovered some crashes when restoring Local/SessionStorage objects.
> 
> The "Storage" object is a wrapper around a StorageArea.
> DOMWindow creates its own Storage object wrapping its LocalStorage area, which gets registered as one of its DOMWindowProperties.
> The WebInspector also creates its own Storage object wrapping the same LocalStorage area which also gets registered as one of the DOMWindow's DOMWindowProperties.
> 
> Then you leave the page and it is cached, and we disconnect all DOMWindowProperties from the frame.
> 
> Then the WebInspector's wrapper is cleaned up.  In the ::~Storage destructor it can't unregister itself from the DOMWindow since it doesn't have its Frame* any long.
> 
> Then you go back, and the DOMWindow tries to reconnect this invalid Storage object.
> 
> I'm working on coming up with a clever solution to this without bloating the DOMWindowProperty mechanism that much.

This is unique to the Storage objects, and is because of what the Inspector is doing with them.  It seems bizarre that the Inspector needs to create a DOMWindowProperty (Storage object) and can't just use the StorageArea directly.  Tim Hatcher agrees.  Filing a new bugzilla about that.

[Brady Eidson]

(In reply to comment #20)
> Filing a new bugzilla about that.

https://bugs.webkit.org/show_bug.cgi?id=80903

[Brady Eidson]

Okay, that little hiccup is solved.  Polishing off a layout test and patch to come shortly.

[Brady Eidson]

Created attachment 131489 [details]
Patch v1 - Fix + layout test 

I'm still testing the edges, but just wanted to get a patch posted before heading home.

[Adam Barth]

Comment on attachment 131489 [details]
Patch v1 - Fix + layout test 

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

This looks great.

> Source/WebCore/Modules/indexeddb/DOMWindowIndexedDatabase.cpp:63
> -    m_idbFactory = 0;
> +    m_suspendedIDBFactory = m_idbFactory.release();

That's a nice approach.

> Source/WebCore/page/DOMWindow.cpp:537
> +#if ENABLE(NOTIFICATIONS)
> +    // FIXME: Notifications shouldn't have different disconnection logic than
> +    // the rest of the DOMWindowProperties.
> +    // There is currently no way to reconnect them in resumeFromPageCache() so
> +    // they will be broken after returning to a cached page.
> +    resetNotifications();
> +#endif

This special case will be removed in https://bugs.webkit.org/show_bug.cgi?id=79636

[WebKit Review Bot]

Comment on attachment 131489 [details]
Patch v1 - Fix + layout test 

Attachment 131489 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11947294

[Brady Eidson]

(In reply to comment #24)
> (From update of attachment 131489 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131489&action=review
> 
> This looks great.
> 
> > Source/WebCore/Modules/indexeddb/DOMWindowIndexedDatabase.cpp:63
> > -    m_idbFactory = 0;
> > +    m_suspendedIDBFactory = m_idbFactory.release();
> 
> That's a nice approach.
> 
> > Source/WebCore/page/DOMWindow.cpp:537
> > +#if ENABLE(NOTIFICATIONS)
> > +    // FIXME: Notifications shouldn't have different disconnection logic than
> > +    // the rest of the DOMWindowProperties.
> > +    // There is currently no way to reconnect them in resumeFromPageCache() so
> > +    // they will be broken after returning to a cached page.
> > +    resetNotifications();
> > +#endif
> 
> This special case will be removed in https://bugs.webkit.org/show_bug.cgi?id=79636

I knew removing it was upcoming work.  I'll the bug number to the FIXME before I land this tomorrow.

[Brady Eidson]

http://trac.webkit.org/changeset/110570

[Rob B]

Thanks guys, is there any way for me to determine when this bug will hit iOS? I'm wondering how we can validate that it is fixed.

[Brady Eidson]

(In reply to comment #28)
> Thanks guys, is there any way for me to determine when this bug will hit iOS? I'm wondering how we can validate that it is fixed.

This bug database is for the WebKit open source project.

iOS is a product of Apple, and you'd have ask them about timelines for their product releases.

[Rob B]

Understood, just saw a lot of @apple.com email addresses and no amount of web searching led me to a bug database for iOS. Thanks, I'll keep digging.

[Brady Eidson]

(In reply to comment #30)
> Understood, just saw a lot of @apple.com email addresses and no amount of web searching led me to a bug database for iOS. Thanks, I'll keep digging.

http://developer.apple.com/ and http://bugreport.apple.com/

[Brady Eidson]

*** Bug 94064 has been marked as a duplicate of this bug. ***