Bug 36665 – [v8] Page Cache does not work on Android

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andrei Popescu]

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.

[Andrei Popescu]

Created attachment 51755 [details]
Implement ScriptCachedFrameData on Android

[WebKit Review Bot]

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.

[Andrei Popescu]

Created attachment 51756 [details]
Implement ScriptCachedFrameData on Android (take 2)

Fixed style, both in the code that I added and in the existing code.

[Adam Barth]

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?

[Adam Barth]

Comment on attachment 51756 [details]
Implement ScriptCachedFrameData on Android (take 2)

See comments above.

[Andrei Popescu]

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

[Andrei Popescu]

Created attachment 51769 [details]
Implement ScriptCachedFrameData on Android (take 3)

[Adam Barth]

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?

[Andrei Popescu]

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

[Ben Murdoch]

Will add newlines as per last review comment and land.

[Ben Murdoch]

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.