Bug 184823 – REGRESSION(r230812): [WPE][GTK] WebKitWebViewSessionState.cpp throws away encoded BackForwardList identifier

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

[Michael Catanzaro]

Since r230812, decodeBackForwardListItemState in WebKitWebViewSessionState.cpp no longer reads the encoded BackForwardList identifier.

First, we should figure out if this is bad. Does the BackForwardList still get deserialized properly? It's not clear to me.

If it's not bad, we should at least stop encoding the identifier, since it's silly to encode something that's never decoded. To avoid the need to change the serialization format, we could just start encoding 0, for example, with a comment to indicate that it should be dropped if we ever need to bump the serialization format.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #0)
> Since r230812, decodeBackForwardListItemState in
> WebKitWebViewSessionState.cpp no longer reads the encoded BackForwardList
> identifier.

Right.

> First, we should figure out if this is bad. Does the BackForwardList still
> get deserialized properly? It's not clear to me.

Nah, it's not bad because WebBackForwardList::restoreFromState() assigns a new identifier to the item without checking the saved one. This was done before as well, so I think we were saving the identifier in the state for nothing. Brady, correct me if I'm wrong.

> If it's not bad, we should at least stop encoding the identifier, since it's
> silly to encode something that's never decoded. To avoid the need to change
> the serialization format, we could just start encoding 0, for example, with
> a comment to indicate that it should be dropped if we ever need to bump the
> serialization format.

It should be easy to handle this change, I think.

[Carlos Garcia Campos]

Created attachment 338868 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

Comment on attachment 338868 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebViewSessionState.cpp:394
> +    GRefPtr<GVariant> variant;
> +    for (unsigned i = 0; sessionStateTypeStringVersions[i]; ++i) {
> +        sessionStateTypeString = sessionStateTypeStringVersions[i];
> +        variant = g_variant_new_from_bytes(G_VARIANT_TYPE(sessionStateTypeString), data, FALSE);
> +        if (g_variant_is_normal_form(variant.get()))
> +            break;
> +        variant = nullptr;
> +    }

You've tested this to ensure it doesn't emit a bunch of warnings or criticals when the type doesn't match?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 338868 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338868&action=review
> 
> > Source/WebKit/UIProcess/API/glib/WebKitWebViewSessionState.cpp:394
> > +    GRefPtr<GVariant> variant;
> > +    for (unsigned i = 0; sessionStateTypeStringVersions[i]; ++i) {
> > +        sessionStateTypeString = sessionStateTypeStringVersions[i];
> > +        variant = g_variant_new_from_bytes(G_VARIANT_TYPE(sessionStateTypeString), data, FALSE);
> > +        if (g_variant_is_normal_form(variant.get()))
> > +            break;
> > +        variant = nullptr;
> > +    }
> 
> You've tested this to ensure it doesn't emit a bunch of warnings or
> criticals when the type doesn't match?

Yes, of course I tested it. My ephy history was successfully encoded with version 1 (2 is tried first) and decoded with 2.

[Carlos Garcia Campos]

(In reply to Carlos Garcia Campos from comment #5)
> (In reply to Michael Catanzaro from comment #4)
> > Comment on attachment 338868 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=338868&action=review
> > 
> > > Source/WebKit/UIProcess/API/glib/WebKitWebViewSessionState.cpp:394
> > > +    GRefPtr<GVariant> variant;
> > > +    for (unsigned i = 0; sessionStateTypeStringVersions[i]; ++i) {
> > > +        sessionStateTypeString = sessionStateTypeStringVersions[i];
> > > +        variant = g_variant_new_from_bytes(G_VARIANT_TYPE(sessionStateTypeString), data, FALSE);
> > > +        if (g_variant_is_normal_form(variant.get()))
> > > +            break;
> > > +        variant = nullptr;
> > > +    }
> > 
> > You've tested this to ensure it doesn't emit a bunch of warnings or
> > criticals when the type doesn't match?
> 
> Yes, of course I tested it. My ephy history was successfully encoded with
> version 1 (2 is tried first) and decoded with 2.

Ok, the opposite, decoded with 1 and encoded with 2 ...

[Carlos Garcia Campos]

Committed r231090: <https://trac.webkit.org/changeset/231090>