Bug 60274 - [Windows WebKit2] Use cookies set in WebKit1
Summary: [Windows WebKit2] Use cookies set in WebKit1
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Windows 7
: P2 Normal
Assignee: Jessie Berlin
URL:
Keywords: InRadar
Depends on: 60445 60660
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-05 09:03 PDT by Jessie Berlin
Modified: 2011-06-18 12:13 PDT (History)
7 users (show)

See Also:


Attachments
Patch (915.48 KB, patch)
2011-05-05 12:56 PDT, Jessie Berlin
no flags Details | Formatted Diff | Diff
Path supplementing the one already reviewed that should fix the crashes on the WK2 bots (1.13 KB, patch)
2011-05-11 16:19 PDT, Jessie Berlin
no flags Details | Formatted Diff | Diff
Patch with a better fix for the crash and with a fix for a loose end (897.37 KB, patch)
2011-05-12 10:21 PDT, Jessie Berlin
sfalken: review+
Details | Formatted Diff | Diff
Patch with a better fix for the crash and with a fix for a loose end (Take 2) (deleted)
2011-05-12 11:53 PDT, Jessie Berlin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jessie Berlin 2011-05-05 09:03:32 PDT
<rdar://problem/8927952>
Comment 1 Jessie Berlin 2011-05-05 12:56:42 PDT
Created attachment 92453 [details]
Patch
Comment 2 Jessie Berlin 2011-05-07 17:57:40 PDT
Comment on attachment 92453 [details]
Patch

Committed in http://trac.webkit.org/changeset/86016
Comment 3 Jessie Berlin 2011-05-07 18:14:45 PDT
This change caused crashes on the WK2 Windows bots, so I had sheriffbot roll it out in http://trac.webkit.org/changeset/86018
Comment 4 Jessie Berlin 2011-05-11 16:19:54 PDT
Created attachment 93207 [details]
Path supplementing the one already reviewed that should fix the crashes on the WK2 bots
Comment 5 Jessie Berlin 2011-05-11 17:14:33 PDT
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
Comment 6 Jessie Berlin 2011-05-12 09:30:57 PDT
There are a few outstanding issues that need addressing.
Comment 7 Jessie Berlin 2011-05-12 10:21:59 PDT
Created attachment 93299 [details]
Patch with a better fix for the crash and with a fix for a loose end
Comment 8 Adam Roben (:aroben) 2011-05-12 11:17:11 PDT
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.
Comment 9 Jessie Berlin 2011-05-12 11:53:11 PDT
Created attachment 93318 [details]
Patch with a better fix for the crash and with a fix for a loose end (Take 2)
Comment 10 Darin Adler 2011-05-12 11:54:43 PDT
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?
Comment 11 Adam Roben (:aroben) 2011-05-12 11:55:41 PDT
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.
Comment 12 Jessie Berlin 2011-05-12 12:06:26 PDT
(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.