[webkit-reviews] review denied: [Bug 127926] Add session support to WebPlatformStrategies : [Attachment 222789] fix test failure

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Martin Hock <mhock at apple.com>'s
request for review:
Bug 127926: Add session support to WebPlatformStrategies
https://bugs.webkit.org/show_bug.cgi?id=127926

Attachment 222789: fix test failure
https://bugs.webkit.org/attachment.cgi?id=222789&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=222789&action=review


You can test these on Mavericks if you comment out private browsing tests in
LayoutTests/platform/mac-wk2/TestExpectations, and also comment out these lines
in WebKitTestRunner:

    WKContextSetUsesNetworkProcess(m_context.get(), true);
    WKContextSetProcessModel(m_context.get(),
kWKProcessModelMultipleSecondaryProcesses);

> Source/WebKit2/Shared/SessionTracker.cpp:100
> +    if (staticSessionMap().contains(sessionID)) {
> +	   storageSessionToID().remove(session(sessionID));
> +	   staticSessionMap().remove(sessionID);
> +    }

I don't think that this is right. remove() doesn't crash when removing a
non-existent key. The crashes that happen mean that sessionID is 0, and
contains() will crash in the same manner.

The right fix will be to make sure that
InjectedBundle::setPrivateBrowsingEnabled() passes a correct (legacy) session
ID.