Bug 60274 – [Windows WebKit2] Use cookies set in WebKit1

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jessie Berlin]

<rdar://problem/8927952>

[Jessie Berlin]

Created attachment 92453 [details]
Patch

[Jessie Berlin]

Comment on attachment 92453 [details]
Patch

Committed in http://trac.webkit.org/changeset/86016

[Jessie Berlin]

This change caused crashes on the WK2 Windows bots, so I had sheriffbot roll it out in http://trac.webkit.org/changeset/86018

[Jessie Berlin]

Created attachment 93207 [details]
Path supplementing the one already reviewed that should fix the crashes on the WK2 bots

[Jessie Berlin]

Comment on attachment 93207 [details]
Path supplementing the one already reviewed that should fix the crashes on the WK2 bots

Recommitted the patch with this fix in http://trac.webkit.org/changeset/86285.

And it passes on the tests! http://build.webkit.org/results/Windows%207%20Release%20(WebKit2%20Tests)/r86285%20(6745)/results.html

[Jessie Berlin]

There are a few outstanding issues that need addressing.

[Jessie Berlin]

Created attachment 93299 [details]
Patch with a better fix for the crash and with a fix for a loose end

[Adam Roben (:aroben)]

Comment on attachment 93299 [details]
Patch with a better fix for the crash and with a fix for a loose end

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

> Source/WebKit2/Shared/WebProcessCreationParameters.cpp:45
> +    , serializedDefaultStorageSessionIsValid(false)

I don't think this extra member is needed. You can still encode and decode the boolean just using local variables. There's no need to pass the boolean around in the parameters struct itself.

[Jessie Berlin]

Created attachment 93318 [details]
Patch with a better fix for the crash and with a fix for a loose end (Take 2)

[Darin Adler]

Comment on attachment 93318 [details]
Patch with a better fix for the crash and with a fix for a loose end (Take 2)

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

> Source/WebKit2/Shared/WebProcessCreationParameters.cpp:166
> -    if (parameters.serializedDefaultStorageSession && !CoreIPC::decode(decoder, parameters.serializedDefaultStorageSession))
> +    bool hasStorageSession = false;
> +    if (!decoder->decode(hasStorageSession))
> +        return false;
> +    if (hasStorageSession && !CoreIPC::decode(decoder, parameters.serializedDefaultStorageSession))
>          return false;

What zeroes out parameters.serializedDefaultStorageSession?

[Adam Roben (:aroben)]

Comment on attachment 93318 [details]
Patch with a better fix for the crash and with a fix for a loose end (Take 2)

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

> Source/WebKit2/Shared/WebProcessCreationParameters.cpp:89
> +    CFDataRef storageSession = serializedDefaultStorageSession.get();
> +    encoder->encodeBool(storageSession);
> +    if (storageSession)
> +        CoreIPC::encode(encoder, storageSession);

You could probably get away without the local storageSession variable. But having it seems fine.

[Jessie Berlin]

(In reply to comment #10)
> (From update of attachment 93318 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93318&action=review
> 
> > Source/WebKit2/Shared/WebProcessCreationParameters.cpp:166
> > -    if (parameters.serializedDefaultStorageSession && !CoreIPC::decode(decoder, parameters.serializedDefaultStorageSession))
> > +    bool hasStorageSession = false;
> > +    if (!decoder->decode(hasStorageSession))
> > +        return false;
> > +    if (hasStorageSession && !CoreIPC::decode(decoder, parameters.serializedDefaultStorageSession))
> >          return false;
> 
> What zeroes out parameters.serializedDefaultStorageSession?

It is a RetainPtr, which is automatically initialized to 0 when the WebProcessCreationParameters is constructed in the Web Process.