Bug 34679

Summary:

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

Product:

WebKit

Reporter:

"Cowboy" Ben Alman <cowboy>

Component:

Page Loading

Assignee:

Brady Eidson <beidson>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, ap, beidson, dglazkov, gojpeg, htengelin, japhet, kangax, mathias, michaeln, rob, ryan, sam, webkit.review.bot

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

URL:

http://benalman.com/code/projects/jquery-hashchange/examples/bug-safari-back-from-diff-domain/

Bug Depends on:

80903

Bug Blocks:

Attachments:

Description	Flags
Example of problem with iframes and iOS 5 b/f cache	none
Patch v1 - Fix + layout test	abarth: review+, webkit.review.bot: commit-queue-

"Cowboy" Ben Alman

Reported 2010-02-06 04:42:08 PST

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.

Attachments
Example of problem with iframes and iOS 5 b/f cache (2.65 KB, application/zip) 2012-02-03 20:13 PST, Rob B	no flags	Details
Patch v1 - Fix + layout test (27.03 KB, patch) 2012-03-12 19:09 PDT, Brady Eidson	abarth: review+ webkit.review.bot: commit-queue-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2010-02-06 23:32:23 PST

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

Comment 2 2010-04-26 14:24:56 PDT

<rdar://problem/7908830>

Alexey Proskuryakov

Comment 3 2011-02-22 16:20:46 PST

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

Adam Barth

Comment 4 2011-10-07 15:08:47 PDT

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

Rob B

Comment 5 2012-02-03 20:13:50 PST

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

Rob B

Comment 6 2012-02-03 20:14:34 PST

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

Comment 7 2012-03-06 15:35:25 PST

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

Comment 8 2012-03-06 15:47:07 PST

(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

Comment 9 2012-03-08 17:00:41 PST

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

Comment 10 2012-03-08 17:08:35 PST

> 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

Comment 11 2012-03-08 17:14:41 PST

(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

Comment 12 2012-03-08 17:38:43 PST

> 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

Comment 13 2012-03-08 18:56:18 PST

(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

Comment 14 2012-03-09 11:41:38 PST

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

Comment 15 2012-03-09 12:55:08 PST

> 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

Comment 16 2012-03-09 13:02:10 PST

(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

Comment 17 2012-03-09 13:02:57 PST

(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

Comment 18 2012-03-09 13:19:33 PST

> 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

Comment 19 2012-03-12 15:43:37 PDT

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

Comment 20 2012-03-12 15:53:44 PDT

(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

Comment 21 2012-03-12 15:56:59 PDT

(In reply to comment #20) > Filing a new bugzilla about that. https://bugs.webkit.org/show_bug.cgi?id=80903

Brady Eidson

Comment 22 2012-03-12 16:35:27 PDT

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

Brady Eidson

Comment 23 2012-03-12 19:09:48 PDT

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 24 2012-03-12 19:15:54 PDT

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 25 2012-03-12 20:27:49 PDT

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

Comment 26 2012-03-12 21:09:10 PDT

(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

Comment 27 2012-03-13 09:00:05 PDT

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

Rob B

Comment 28 2012-08-15 13:55:26 PDT

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

Comment 29 2012-08-15 14:28:47 PDT

(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

Comment 30 2012-08-15 19:22:52 PDT

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

Comment 31 2012-08-16 09:29:15 PDT

(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

Comment 32 2012-08-16 09:34:41 PDT

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

Note You need to log in before you can comment on or make changes to this bug.