Summary: | Plug-ins don't always respect the cookie accept policy | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeff Johnson <opendarwin> | ||||||||||
Component: | New Bugs | Assignee: | Brady Eidson <beidson> | ||||||||||
Status: | VERIFIED FIXED | ||||||||||||
Severity: | Normal | CC: | andersca, ap, beidson, eric, lutzp, rachael | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
URL: | http://www.time.com/time/world/article/0,8599,1904577,00.html | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 35905 | ||||||||||||
Attachments: |
|
Description
Jeff Johnson
2009-06-14 15:18:59 PDT
Typo in the notes: it should say "The method setCookies:forURL:mainDocumentURL: does get called frequently, ALWAYS from setCookies() in WebKit" rather than "The method setCookies:forURL:mainDocumentURL: does get called frequently, ALSO from setCookies() in WebKit" In my testing, it seems that the cookie is set when the page loads the application/x-shockwave-flash URL http://widgets.clearspring.com/o/475db72337b302d4/-/-/-/ The URL request is created in -[WebNetscapePluginView loadStream]. The request by default has HTTPShouldHandleCookies YES. Then in ResourceRequest::doUpdatePlatformRequest() it calls [nsRequest setMainDocumentURL:firstPartyForCookies()], but firstPartyForCookies() returns nil. *** Bug 9445 has been marked as a duplicate of this bug. *** (In reply to comment #2) > <rdar://problem/6971214> Using <rdar://problem/7338359> to track this now Created attachment 50280 [details]
WIP fix
Work in progress.
Almost done, actually. I'm still thinking through the ResourceLoader level being the right place where the firstPartyForCookies url is set.
Currently FrameLoader handles this in ::addExtraFieldsToRequest() before handing the request off to SubresourceLoader and MainResourceLoader, which both then pipe down to ResourceLoader.
It's kind of sad that the FrameLoader::addExtraFieldsToRequest() doesn't happen at the ResourceLoader level itself but I don't know that simply moving it is a great idea.
I'll decide something tomorrow and hopefully put it up for review in the morning.
Created attachment 50321 [details]
Don't start a ResourceLoader with a null firstPartyForCookies
After sleeping on it, I think this is the right way to go for now. It's a shame that plugin code doesn't go through FrameLoader, and therefore doesn't automatically get the extra fields added.
Since plug-in code is currently so strewn about and very platform specific, I think reworking it needs to be a task for that domain's expert. I don't think there's any harm in this targeted fix for the hole in our cookie accept policy until that can happen.
Comment on attachment 50321 [details]
Don't start a ResourceLoader with a null firstPartyForCookies
Better changelog coming
Created attachment 50322 [details]
Better ChangeLog
Comment on attachment 50322 [details]
Better ChangeLog
This needs a "why" comment. The change log explains it well but the code needs something too.
r=me
http/tests/cookies/third-party-cookie-relaxing.html -> failed failed on the commit-bot when doing the "does trunk actually build and test" check before trying to commit. I believe that failure is due to this checkin moments ago and that the bots will soon roll red. :( (In reply to comment #13) > http/tests/cookies/third-party-cookie-relaxing.html -> failed > failed on the commit-bot when doing the "does trunk actually build and test" > check before trying to commit. > > I believe that failure is due to this checkin moments ago and that the bots > will soon roll red. :( Looking at the layouttest failures, it's apparently that the various cookie tests don't reset themselves to a consistent state. Amazing we didn't have problems like this with cookie tests in the past. I will try to fix the two tests I've added recently that have gone red because of this. (In reply to comment #14) > (In reply to comment #13) > > http/tests/cookies/third-party-cookie-relaxing.html -> failed > > failed on the commit-bot when doing the "does trunk actually build and test" > > check before trying to commit. > > > > I believe that failure is due to this checkin moments ago and that the bots > > will soon roll red. :( > > Looking at the layouttest failures, it's apparently that the various cookie > tests don't reset themselves to a consistent state. Amazing we didn't have > problems like this with cookie tests in the past. I will try to fix the two > tests I've added recently that have gone red because of this. There were some cookie utility functions that were used by *SOME* cookie tests to clear the cookie state. It doesn't seem to be used consistently. I'm using it before and after each of these two new tests to keep things in a consistent state. Created attachment 50341 [details]
Quick review, perhaps?
Comment on attachment 50341 [details]
Quick review, perhaps?
r=me
(In reply to comment #18) > http://trac.webkit.org/changeset/55742 That fix apparently didn't work. I still don't know why not. Clearly these tests are leaking cookies, causing the failure. I just don't know why the bots are seeing this and I can't reproduce here. SnowLeopard release bot and Leopard debug bots are clear. I'm still lost as to why Tiger and Leopard release are still failing these tests. Verified as fixed with git commit 83853ec7af2cf0f4937a743fe99371b6592017b1 svn r55820. The original time.com URL is no longer demonstrating the original bug with stock 10.6.2 WebKit, but I found another web site that does reproduce the original bug with 10.6.2 WebKit. Fixed in the trunk. And for full closure on the failing test issue, we made changes to *really* keep the tests consistent and they seem to have stuck. *** Bug 26880 has been marked as a duplicate of this bug. *** |