Bug 115600 – [GTK] Allow to save and restore session

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Daniel Narvaez]

As pointed out in bug 69358, there is no way to restore only back/forward history in WebKit2 but it's possible to save and restore the whole session state (which in my opinion is much better). We should bind the two APIs in gtk.

WK_EXPORT WKDataRef WKPageCopySessionState(WKPageRef page, void* context, WKPageSessionStateFilterCallback urlAllowedCallback);
WK_EXPORT void WKPageRestoreFromSessionState(WKPageRef page, WKDataRef sessionStateData);

[Sam P.]

Newer discussion of this API:  https://bugs.webkit.org/show_bug.cgi?id=147525  https://bugs.webkit.org/show_bug.cgi?id=69343#c23

[Sam P.]

Created attachment 261304 [details]
Patch

[Sam P.]

As a little demo test, you can change BrowserWindow.c in minibrowser to have reloadOrStopCallback like:

    GBytes * state = NULL;
    /* Horrible style, just a demo */
    static void reloadOrStopCallback(BrowserWindow *window)
    {
        if (!state) {
            state = webkit_web_view_serialize_session(window->webView);
        } else {
            webkit_web_view_restore_session(window->webView, state, TRUE);
            state = NULL;
        }
    }

Then when you run minibrowser, press the stop button to save the session state.  Then navigate around the web to somewhere else.  Press the stop button again and you should be back to where you saved the state and the back forward lists, etc. should remain the same.

[Carlos Garcia Campos]

Comment on attachment 261304 [details]
Patch

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

Thanks for the patch. This adds new API, so it probably requires some discussion in the mailing list first. The patch should include unit tests to cover the new API added. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API. If we are going to expose the session state API, I think we should first implement the internal implementation and enabling the cross-platform unit tests to make sure it works. I think that adding a LegacySessionStateCoding implementation could be enough, and then enable the Tests in Tools/TestWebKitAPI/Tests/WebKit2 that use the WKSession API. We could use bug #84960 for that part. Once that is working, then we can expose it in our API.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3717
> + * webkit_web_view_serialize_session

Missing trailing : here

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3724
> + * Returns: (transfer none): A GBytes buffer containing the state, or NULL.

This says transfer none, but returns a newly created GBytes that we don't free. 
GBytes -> #GBytes
NULL -> %NULL

We should also document why this can return NULL.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3726
> + * Since: 2.10

Too late for 2.10, this should be 2.12

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3728
> +GBytes* webkit_web_view_serialize_session(WebKitWebView *webView)

WebKitWebView *webView -> WebKitWebView* webView

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3730
> +    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL);

NULL -> nullptr

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3733
> +    g_return_val_if_fail(!!page, NULL);

Don't ue g_return macros for this. Use an ASSERT or and early return.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3735
> +    SessionState x = page->sessionState(0);

Do not use x for a variable name, use a word that better defines what this is for. And use nullptr instead o 0. See the WebKit coding style: http://www.webkit.org/coding/coding-style.html

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3740
> +    GBytes* buf = g_bytes_new_take(encoder->buffer(), encoder->bufferSize());
> +    return buf;

We don't need the local variable. The ArgumentEncoder previously allocated is leaked here. And the data can't be taken by the GBytes, because it's owned by the ArgumentEncoder and because it will be freed by the GBytes using g_free, but was allocated by WebKit using bmalloc. In any case, I don't think we should use the ArgumentEncoder here this way. We should either return an object wrapping the SessionState, or if we really want to return encoded data, use the encodeLegacySessionState() method.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3744
> + * webkit_web_view_restore_session

Missing trailing : here

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3747
> + * @navigate: a #gboolean that if #TRUE will trigger navigation to the site set in the state

#TRUE -> %TRUE

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3751
> + * Since: 2.10

Too late for 2.10, this should be 2.12

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3753
> +void webkit_web_view_restore_session(WebKitWebView *webView, GBytes *state, gboolean navigate)

WebKitWebView *webView -> WebKitWebView* webView
GBytes *state -> GBytes* state

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3758
> +    WebPageProxy* page = getPage(webView);
> +    g_return_if_fail(!!page);

Don't ue g_return macros for this. Use an ASSERT or and early return.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3762
> +        (uint8_t *) g_bytes_get_data(state, &size), size);

Use C++ style casts.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3764
> +    SessionState session_state;

session_state -> sessionState

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3766
> +    page->restoreFromSessionState(session_state, navigate);

WTF::move(session_state)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3767
> +}

The decoder is also leaked here.

[Sam P.]

Thanks for the in depth review.  I will look into the legacy state encoder - it seems to fit my needs better than this patch.

>  If we are going to expose the session state API, I think we should first implement the internal implementation and enabling the cross-platform unit tests to make sure it works.  I think that adding a LegacySessionStateCoding implementation could be enough, and then enable the Tests in Tools/TestWebKitAPI/Tests/WebKit2 that use the WKSession API. We could use bug #84960 for that part. Once that is working, then we can expose it in our API.

What do you mean by the internal implementation?  A change to WebKitWebView.c or something more internal?

[Carlos Garcia Campos]

Created attachment 266798 [details]
Patch

Some notes about the API:

 - It adds WebKitWebViewSessionState, a boxed type that can be created by the web view to get the current session state, or by the user from serialized data.
 - webkit_web_view_get_session_state() is transfer full because the session state is something that changes all the time, so you get the current state, it's not something we want to cache.
 - webkit_web_view_get_session_state() always passes nullptr as filter to WebPageProxy::sessionState. I'm not sure why the user would want to filter the bf list, so for now I left it that way, if we eventually want to add support for that we can add webkit_web_view_get_filtered_session_state() or something similar.

[Anders Carlsson]

Comment on attachment 266798 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewSessionState.cpp:136
> +    g_return_val_if_fail(state, nullptr);
> +    IPC::ArgumentEncoder encoder;
> +    encoder << state->sessionState.backForwardListState;
> +    encoder << state->sessionState.renderTreeSize;
> +    encoder << state->sessionState.provisionalURL;
> +    return g_bytes_new(encoder.buffer(), encoder.bufferSize());

NO.

Don't do this, we DON'T want to rely on the IPC encoding for this.

[Carlos Garcia Campos]

(In reply to comment #7)
> Comment on attachment 266798 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266798&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewSessionState.cpp:136
> > +    g_return_val_if_fail(state, nullptr);
> > +    IPC::ArgumentEncoder encoder;
> > +    encoder << state->sessionState.backForwardListState;
> > +    encoder << state->sessionState.renderTreeSize;
> > +    encoder << state->sessionState.provisionalURL;
> > +    return g_bytes_new(encoder.buffer(), encoder.bufferSize());
> 
> NO.
> 
> Don't do this, we DON'T want to rely on the IPC encoding for this.

Ok, I simply didn't know it. What's the right way then? Should I use my own encoding/decoding thing?

[Anders Carlsson]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 266798 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=266798&action=review
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewSessionState.cpp:136
> > > +    g_return_val_if_fail(state, nullptr);
> > > +    IPC::ArgumentEncoder encoder;
> > > +    encoder << state->sessionState.backForwardListState;
> > > +    encoder << state->sessionState.renderTreeSize;
> > > +    encoder << state->sessionState.provisionalURL;
> > > +    return g_bytes_new(encoder.buffer(), encoder.bufferSize());
> > 
> > NO.
> > 
> > Don't do this, we DON'T want to rely on the IPC encoding for this.
> 
> Ok, I simply didn't know it. What's the right way then? Should I use my own
> encoding/decoding thing?

Yes. Preferably something that can be extended without breaking the format.

FWIW, the reason we don't want this is because it means we can't change the IPC format without breaking compat. We did this on OS X in the past and it caused us lots of pain.

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Comment on attachment 266798 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=266798&action=review
> > > 
> > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewSessionState.cpp:136
> > > > +    g_return_val_if_fail(state, nullptr);
> > > > +    IPC::ArgumentEncoder encoder;
> > > > +    encoder << state->sessionState.backForwardListState;
> > > > +    encoder << state->sessionState.renderTreeSize;
> > > > +    encoder << state->sessionState.provisionalURL;
> > > > +    return g_bytes_new(encoder.buffer(), encoder.bufferSize());
> > > 
> > > NO.
> > > 
> > > Don't do this, we DON'T want to rely on the IPC encoding for this.
> > 
> > Ok, I simply didn't know it. What's the right way then? Should I use my own
> > encoding/decoding thing?
> 
> Yes. Preferably something that can be extended without breaking the format.
> 
> FWIW, the reason we don't want this is because it means we can't change the
> IPC format without breaking compat. We did this on OS X in the past and it
> caused us lots of pain.

Ok, understood, thanks for the explanation. I'll rework it.

[Carlos Garcia Campos]

Created attachment 268006 [details]
Updated patch

It uses GVariant instead of IPC encoders/decoders.

[Carlos Garcia Campos]

Created attachment 268007 [details]
Rebased to apply on current trunk

[Michael Catanzaro]

Comment on attachment 268007 [details]
Rebased to apply on current trunk

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

This gets API approval from me, but please propose it on the mailing list and wait a few days anyway, to give folks a chance to object, even though probably nobody will.

I almost gave this r- for the messy implementation, since it took me a long time to accept that a GVariant of type (qa(ts(ssssasmaytt(ii)dm(sa(uaysxmxmds))av)u)mu) is really the best way to handle this, rather than using nice C++ iostreams like in your previous patch... but Anders is right, we can't use the IPC encoders because our serialization would break whenever the implementation changes. We hardly want to reimplement all the iostream encoders in this file, which is what it would take to use iostreams safely. It's unfortunate that GVariant is so much harder to use than iostreams, but the mess is contained into one file, and you split it up nicely into "nested" functions. And with GVariant, we know the serialization format is not going to change.

My biggest issue with this patch is that we should have some way to detect changes to the implementation of HTTPBody, FrameState, PageState, BackForwardListState, so that we know to update the serialization here. Currently we can detect deletions, since we'd get a compiler error, but if any new state is added to these classes we will miss it. I think you can solve this using messy static_asserts and sizeof to check the size of the structs. Something like:

#if CPU(X86_64) && COMPILER(GCC_OR_CLANG) && DEVELOPER_MODE
static_assert(sizeof(HTTPBody::Element == sizeof(Type) + sizeof(Vector<char>) + sizeof(String) + sizeof(int64_t) + sizeof(Optional<int64_t>) + sizeof(Optional<double>) + sizeof(String));
#endif

I guess that is pretty messy, though.

> Source/WebKit2/ChangeLog:15
> +        session state form the given WebKitWebViewSessionState.

form -> rom

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3754
> +    getPage(webView)->restoreFromSessionState(webkitWebViewSessionStatateGetSessionState(state), false);

You need to fix this typo all over. webkitWebViewSessionStatateGetSessionState -> webkitWebViewSessionStateGetSessionState

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewSessionState.cpp:59
> +static inline unsigned toExternalURLsPolicy(WebCore::ShouldOpenExternalURLsPolicy policy)

I almost thought you did not need these enums (ExternalURLsPolicy, HTMLBodyElementType) or the conversion functions to go with them. After all, you could just do e.g. static_cast<unsigned>(myWebCoreEnumValue). Deleting them would save about 70 lines of code. Of course, you do need them to guarantee the serialization format, in case the WebCore enums get reordered or something. I recommend adding a comment, else someone like me might break this as a "code cleanup."

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestBackForwardList.cpp:285
> +    webkit_web_view_session_state_unref(state);

OK, but this reminds me, you forgot to declare an autocleanup in WebKitAutocleanups.h.

[Michael Catanzaro]

(In reply to comment #13)
> #if CPU(X86_64) && COMPILER(GCC_OR_CLANG) && DEVELOPER_MODE
> static_assert(sizeof(HTTPBody::Element == sizeof(Type) +
> sizeof(Vector<char>) + sizeof(String) + sizeof(int64_t) +
> sizeof(Optional<int64_t>) + sizeof(Optional<double>) + sizeof(String));
> #endif

You would then need to put __attribute__((__packed__)) on the structs, in an #ifdef, and get an owner to OK it. Maybe check && NDEBUG as well to make sure not to slow down release builds.

I think it'd be worth it, to make sure we don't miss serializing new values.

> > Source/WebKit2/ChangeLog:15
> > +        session state form the given WebKitWebViewSessionState.
> 
> form -> rom

form -> from

[Carlos Garcia Campos]

(In reply to comment #13)
> Comment on attachment 268007 [details]
> Rebased to apply on current trunk
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268007&action=review

Thanks for the review.

> This gets API approval from me, but please propose it on the mailing list
> and wait a few days anyway, to give folks a chance to object, even though
> probably nobody will.

Well, we normally do it the other way around, we propose a new API and once we agree on it in the mailing list we write a patch. Once the patch is written, people can comment here about the API or the implementation.

> I almost gave this r- for the messy implementation, since it took me a long
> time to accept that a GVariant of type
> (qa(ts(ssssasmaytt(ii)dm(sa(uaysxmxmds))av)u)mu) is really the best way to
> handle this, rather than using nice C++ iostreams like in your previous
> patch... but Anders is right, we can't use the IPC encoders because our
> serialization would break whenever the implementation changes. We hardly
> want to reimplement all the iostream encoders in this file, which is what it
> would take to use iostreams safely. It's unfortunate that GVariant is so
> much harder to use than iostreams, but the mess is contained into one file,
> and you split it up nicely into "nested" functions. And with GVariant, we
> know the serialization format is not going to change.

I agree, because it also took me a long time to get this right with GVariant, which is hard to use and even more to debug for complex data like this one.

> My biggest issue with this patch is that we should have some way to detect
> changes to the implementation of HTTPBody, FrameState, PageState,
> BackForwardListState, so that we know to update the serialization here.

Well, it's not that problematic, we only read and write the members we already know about. If something new is added, I assume it would not break anything if you don't use it, I don't know. But every time that happens we will need to bump the version and support both the current one and the new one, so I hope that doesn't happen often.

> Currently we can detect deletions, since we'd get a compiler error, but if
> any new state is added to these classes we will miss it. I think you can
> solve this using messy static_asserts and sizeof to check the size of the
> structs. Something like:
> 
> #if CPU(X86_64) && COMPILER(GCC_OR_CLANG) && DEVELOPER_MODE
> static_assert(sizeof(HTTPBody::Element == sizeof(Type) +
> sizeof(Vector<char>) + sizeof(String) + sizeof(int64_t) +
> sizeof(Optional<int64_t>) + sizeof(Optional<double>) + sizeof(String));
> #endif
> 
> I guess that is pretty messy, though.

Yes. I would rather create a watchlist rule to watch patches changing SessionState.h :-P

> > Source/WebKit2/ChangeLog:15
> > +        session state form the given WebKitWebViewSessionState.
> 
> form -> rom

Oops.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3754
> > +    getPage(webView)->restoreFromSessionState(webkitWebViewSessionStatateGetSessionState(state), false);
> 
> You need to fix this typo all over.
> webkitWebViewSessionStatateGetSessionState ->
> webkitWebViewSessionStateGetSessionState

:-D

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewSessionState.cpp:59
> > +static inline unsigned toExternalURLsPolicy(WebCore::ShouldOpenExternalURLsPolicy policy)
> 
> I almost thought you did not need these enums (ExternalURLsPolicy,
> HTMLBodyElementType) or the conversion functions to go with them. After all,
> you could just do e.g. static_cast<unsigned>(myWebCoreEnumValue). Deleting
> them would save about 70 lines of code. Of course, you do need them to
> guarantee the serialization format, in case the WebCore enums get reordered
> or something. I recommend adding a comment, else someone like me might break
> this as a "code cleanup."

Ok. 

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestBackForwardList.cpp:285
> > +    webkit_web_view_session_state_unref(state);
> 
> OK, but this reminds me, you forgot to declare an autocleanup in
> WebKitAutocleanups.h.

Yep, we should update http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API to suggest adding autoptrs for new public objects/boxed types.

[Michael Catanzaro]

(In reply to comment #15) 
> Well, we normally do it the other way around, we propose a new API and once
> we agree on it in the mailing list we write a patch. Once the patch is
> written, people can comment here about the API or the implementation.

I would post it anyway; I'm pretty sure this API is ideal, but you never know if someone might suggest an improvement.

> > I almost gave this r- for the messy implementation, since it took me a long
> > time to accept that a GVariant of type
> > (qa(ts(ssssasmaytt(ii)dm(sa(uaysxmxmds))av)u)mu) is really the best way to
> > handle this, rather than using nice C++ iostreams like in your previous
> > patch...

To be clear, this is nuts, but I can't think of any better way to do it, and I really want the API. ;)

> Yes. I would rather create a watchlist rule to watch patches changing
> SessionState.h :-P

OK, that's fine too.

> Yep, we should update
> http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API to suggest adding
> autoptrs for new public objects/boxed types.

Done.

[Michael Catanzaro]

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

[Martin Robinson]

Comment on attachment 268007 [details]
Rebased to apply on current trunk

The API looks good to me as well.

[Carlos Garcia Campos]

Committed r194579: <http://trac.webkit.org/changeset/194579>