When enabling the page cache on Android and using V8, the browser crashes when navigating back in the history list. This is because the V8 bindings specific implementation of ScriptCachedFrameData is just a stub. Patch for fixing this for Android coming soon.
Created attachment 51755 [details] Implement ScriptCachedFrameData on Android
Attachment 51755 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/ChangeLog:15: Line contains tab character. [whitespace/tab] [5] WebCore/bindings/v8/ScriptCachedFrameData.h:67: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 51756 [details] Implement ScriptCachedFrameData on Android (take 2) Fixed style, both in the code that I added and in the existing code.
Comment on attachment 51755 [details] Implement ScriptCachedFrameData on Android >+ Note that isolated worlds are not taken into account in any way, Tab? >+ * Copyright 2010, The Android Open Source Project I'm not sure we can accept this sort of copyright line. We've been told not to accept "the chromium authors". Not sure how that applies here. >+#if PLATFORM(ANDROID) Why is this android specific? Seems like anyone who wants to use page cache and v8 will want this. >+ScriptCachedFrameData::ScriptCachedFrameData(Frame* frame) : m_domWindow(frame->domWindow()) Please move initializer to the next line. >+ m_context->ReattachGlobal(m_global); I'm worried that m_global might be the global object from an isolated world. Do we understand how this code interacts with isolated worlds? We might want something either here or when we grab the global to make sure that we're getting the main world's object. >- * Copyright (C) 2008, 2009 Google Inc. All rights reserved. >+ * Copyright (C) 2008, 2009, 2010 Google Inc. All rights reserved. You just need the latest year. >+#if PLATFORM(CHROMIUM) > // We don't use WebKit's page caching, so this implementation is just a stub. Gain, this had nothing to do with CHROMIUM / ANDROID. There's already a bit somewhere that says whether PageCache is enabled. We should key off of that. >+// FIXME: the right guard should be ENABLE(PAGE_CACHE). Replace with the right guard, once >+// https://bugs.webkit.org/show_bug.cgi?id=35061 is fixed. >+// >+// On Android we do use WebKit's page cache. For now we don't support isolated worlds >+// so our implementation does not take them into account. Ah, I see. Can you add some nasty ASSERTS about this so we don't turn this on by accident and not realize the mess we've gotten ourselves into? >+ v8::Persistent<v8::Object> m_global; >+ v8::Persistent<v8::Context> m_context; Please use OwnHandle so you don't have to manually alloc and free these persistent handles. >+void V8DOMWindowShell::setContext(v8::Handle<v8::Context> context) >+{ >+ m_context = v8::Persistent<v8::Context>::New(context); >+} Does this leak the persistent handle to the old m_context? Do we need to ASSERT that m_context is always empty here? Should we use OwnHandle to help us manage the lifetime of the persistent handle?
Comment on attachment 51756 [details] Implement ScriptCachedFrameData on Android (take 2) See comments above.
Hi Adam, Many thanks for the quick reply. (In reply to comment #4) > (From update of attachment 51755 [details]) > >+ Note that isolated worlds are not taken into account in any way, > > Tab? > Fixed. > >+ * Copyright 2010, The Android Open Source Project > > I'm not sure we can accept this sort of copyright line. We've been told not to > accept "the chromium authors". Not sure how that applies here. > Well, I am from Android and our previous contributions all had the same copyright line. For example see: http://trac.webkit.org/browser/trunk/WebCore/page/GeolocationPositionCache.cpp http://trac.webkit.org/browser/trunk/WebCore/page/Geolocation.cpp Given these and other precedents, I think it should be fine to keep the current copyright? > >+#if PLATFORM(ANDROID) > > Why is this android specific? Seems like anyone who wants to use page cache > and v8 will want this. > Ok, removed. > >+ScriptCachedFrameData::ScriptCachedFrameData(Frame* frame) : m_domWindow(frame->domWindow()) > > Please move initializer to the next line. > Moved. > >+ m_context->ReattachGlobal(m_global); > > I'm worried that m_global might be the global object from an isolated world. > Do we understand how this code interacts with isolated worlds? We might want > something either here or when we grab the global to make sure that we're > getting the main world's object. > True, although I'd stay away from that until Android starts using isolated worlds. Else, if Chromium want page cache, somebody who understand isolated worlds better than me should do this change? > >- * Copyright (C) 2008, 2009 Google Inc. All rights reserved. > >+ * Copyright (C) 2008, 2009, 2010 Google Inc. All rights reserved. > > You just need the latest year. > Fixed. > >+#if PLATFORM(CHROMIUM) > > // We don't use WebKit's page caching, so this implementation is just a stub. > > Gain, this had nothing to do with CHROMIUM / ANDROID. There's already a bit > somewhere that says whether PageCache is enabled. We should key off of that. > Hmm, I looked but all I found was a bug for it: https://bugs.webkit.org/show_bug.cgi?id=35061 It's also possible that I missed something. If so, please let me know. > >+// FIXME: the right guard should be ENABLE(PAGE_CACHE). Replace with the right guard, once > >+// https://bugs.webkit.org/show_bug.cgi?id=35061 is fixed. > >+// > >+// On Android we do use WebKit's page cache. For now we don't support isolated worlds > >+// so our implementation does not take them into account. > > Ah, I see. Can you add some nasty ASSERTS about this so we don't turn this on > by accident and not realize the mess we've gotten ourselves into? > Good point. Added a scary assert, it holds on Android but haven't tested on Chromium. Please have a look, I can add more if you have any suggestions. > >+ v8::Persistent<v8::Object> m_global; > >+ v8::Persistent<v8::Context> m_context; > > Please use OwnHandle so you don't have to manually alloc and free these > persistent handles. > Oh, I didn't know about that one, thanks! > >+void V8DOMWindowShell::setContext(v8::Handle<v8::Context> context) > >+{ > >+ m_context = v8::Persistent<v8::Context>::New(context); > >+} > > Does this leak the persistent handle to the old m_context? Do we need to > ASSERT that m_context is always empty here? Should we use OwnHandle to help us > manage the lifetime of the persistent handle? Well spotted, it won't always be empty. And indeed, I added the ASSERT and it doesn't hold. I think it's better to clear the previous context manually and stick to using a persistent handle. If we move to an OwnHandle, this change becomes quite intrusive as I would have to change about 30 sites to call get() instead of accessing m_context directly. But if you really think OwnHandle is better, I can do that. Many thanks, Andrei
Created attachment 51769 [details] Implement ScriptCachedFrameData on Android (take 3)
Comment on attachment 51769 [details] Implement ScriptCachedFrameData on Android (take 3) + namespace WebCore { We usually like a blank line after the namespace declaration. I don't see restore() or clear() called anywhere. Maybe that's for a future patch?
Thanks a lot for the r+ :) (In reply to comment #8) > (From update of attachment 51769 [details]) > + namespace WebCore { > > We usually like a blank line after the namespace declaration. > > I don't see restore() or clear() called anywhere. Maybe that's for a future > patch? Oh, they are called already by WebCore's page cache mechanism: http://trac.webkit.org/browser/trunk/WebCore/history/CachedFrame.cpp#L85 and http://trac.webkit.org/browser/trunk/WebCore/history/CachedFrame.cpp#L196 Thanks, Andrei
Will add newlines as per last review comment and land.
Sending WebCore/ChangeLog Adding WebCore/bindings/v8/ScriptCachedFrameData.cpp Sending WebCore/bindings/v8/ScriptCachedFrameData.h Sending WebCore/bindings/v8/V8DOMWindowShell.cpp Sending WebCore/bindings/v8/V8DOMWindowShell.h Transmitting file data ..... Committed revision 56716.