Bug 52384 - Plumb mixed script URL to FrameLoaderClient
Summary: Plumb mixed script URL to FrameLoaderClient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Langley
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-13 11:25 PST by Adam Langley
Modified: 2011-02-03 21:30 PST (History)
6 users (show)

See Also:


Attachments
Patch (18.47 KB, patch)
2011-01-13 11:26 PST, Adam Langley
no flags Details | Formatted Diff | Diff
Patch (30.12 KB, patch)
2011-01-13 12:46 PST, Adam Langley
no flags Details | Formatted Diff | Diff
Patch (27.38 KB, patch)
2011-01-19 15:05 PST, Adam Langley
no flags Details | Formatted Diff | Diff
Patch (27.39 KB, patch)
2011-01-19 16:20 PST, Adam Langley
no flags Details | Formatted Diff | Diff
Patch (26.66 KB, patch)
2011-01-31 16:21 PST, Adam Langley
no flags Details | Formatted Diff | Diff
updated targetURL to insecureURL (26.70 KB, patch)
2011-02-03 11:55 PST, Adam Langley
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Langley 2011-01-13 11:25:57 PST
Plumb mixed script URL to FrameLoaderClient
Comment 1 Adam Langley 2011-01-13 11:26:24 PST
Created attachment 78833 [details]
Patch
Comment 2 Early Warning System Bot 2011-01-13 12:01:49 PST
Attachment 78833 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7507019
Comment 3 Adam Langley 2011-01-13 12:46:39 PST
Created attachment 78845 [details]
Patch
Comment 4 Adam Langley 2011-01-13 14:03:11 PST
Comment on attachment 78845 [details]
Patch

I'm terribly worried that I've missed updating some override site because the compiler doesn't warn about that. Although I believe that the tests will catch it.

I've deliberately not bothered to plumb this into the ObjC and Windows IDL code because I didn't need to get this value there. However, if there's a general policy that the functions should match then I can do.

cq- because I've still to find a mac to run the layout tests on.
Comment 5 Adam Barth 2011-01-13 14:33:21 PST
Comment on attachment 78845 [details]
Patch

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

This looks fine, module changing String -> URL and the extra changelog entries.  It's fine not to plumb through objective C.  That's an API choice for that port.  We'll also need fishd to review the WebKit API change.

> Tools/ChangeLog:12
> +2011-01-13  Adam Langley  <agl@chromium.org>

You've got two changelog entries.

> Tools/DumpRenderTree/chromium/WebViewHost.h:202
> -    virtual void didRunInsecureContent(WebKit::WebFrame*, const WebKit::WebSecurityOrigin&);
> +    virtual void didRunInsecureContent(WebKit::WebFrame*, const WebKit::WebSecurityOrigin&, const WebKit::WebString& target_url);

I tink we use webkit-style names here.

> WebKit/chromium/public/WebFrameClient.h:279
> -    virtual void didRunInsecureContent(WebFrame*, const WebSecurityOrigin&) { }
> +    virtual void didRunInsecureContent(WebFrame*, const WebSecurityOrigin&, const WebString&) { }

As I commented in the Chromium bug, this should probably be a WebURL.
Comment 6 Alexey Proskuryakov 2011-01-13 21:48:22 PST
This is not a refactoring - how can it be "Covered by http/tests/security/mixedContent/*"? If I understand the patch correctly, the explanation is that is doesn't change behavior, only allowing for better error reporting in the client. 

Ideally, DumpRenderTree for chromium would be enhanced accordingly, and there would be a regression test.
Comment 7 Adam Langley 2011-01-19 14:59:50 PST
(In reply to comment #6)
> This is not a refactoring - how can it be "Covered by http/tests/security/mixedContent/*"? If I understand the patch correctly, the explanation is that is doesn't change behavior, only allowing for better error reporting in the client.

The danger of this patch is that, by changing the underlying method signature, overrides will no longer function. That case should be caught by the tests.

> Ideally, DumpRenderTree for chromium would be enhanced accordingly, and there would be a regression test.

I can do something like the following in Chromium's DRT:

printf("didRunInsecureContent %s://%s:%d %s\n", origin.protocol().utf8().data(), origin.host().utf8().data(), origin.port(), targetURL.spec().data());

But that results in output like:

didRunInsecureContent https://127.0.0.1:8443 http://127.0.0.1:8080/security/resources/redir.php?url=https://127.0.0.1:8443/security/mixedContent/resources/script.js

I could omit printing the origin, but the URL is still going to encode the IP address and port. These are things that I don't think should be in the test expectations but I'll do whatever is advised.
Comment 8 Adam Langley 2011-01-19 15:05:50 PST
Created attachment 79496 [details]
Patch
Comment 9 Build Bot 2011-01-19 15:40:12 PST
Attachment 79496 [details] did not build on win:
Build output: http://queues.webkit.org/results/7496227
Comment 10 Early Warning System Bot 2011-01-19 15:42:00 PST
Attachment 79496 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7539231
Comment 11 Adam Langley 2011-01-19 16:20:02 PST
Created attachment 79515 [details]
Patch
Comment 12 Eric Seidel (no email) 2011-01-19 21:26:50 PST
Attachment 79515 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7633082
Comment 13 Adam Langley 2011-01-20 08:39:18 PST
(In reply to comment #12)
> Attachment 79515 [details] did not build on mac:
> Build output: http://queues.webkit.org/results/7633082

This appears to be a flake. I've built the patch on OS X and all layout tests pass.
Comment 14 Adam Barth 2011-01-20 11:00:57 PST
Looks good to me, but we'll need fishd to give the official review because this changes the Chromium WebKit API.
Comment 15 Eric Seidel (no email) 2011-01-20 13:40:40 PST
Yes, you're right:
/Developer/usr/bin/../libexec/gcc/i686-apple-darwin10/4.2.1/as: can't fork a new process to execute: /Developer/usr/bin/../libexec/gcc/darwin/x86_64/as (Resource temporarily unavailable)
/Projects/MacEWS/Source/WebKit2/WebProcess/Authentication/AuthenticationManager.cpp:104: fatal error: error writing to -: Broken pipe

That's concerning.
Comment 16 Darin Fisher (:fishd, Google) 2011-01-31 15:17:31 PST
Comment on attachment 79515 [details]
Patch

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

Chromium WebKit API changes LGTM modulo this one nit.

> Source/WebKit/chromium/public/WebFrameClient.h:279
> +    virtual void didRunInsecureContent(WebFrame*, const WebSecurityOrigin&, const WebURL&) { }

can you give this WebURL parameter a descriptive name?  it is not obvious
what it refers to.  targetURL seems good, or insecureURL might be better.
Comment 17 Adam Langley 2011-01-31 16:21:33 PST
Created attachment 80698 [details]
Patch
Comment 18 Adam Barth 2011-02-01 11:21:50 PST
Comment on attachment 80698 [details]
Patch

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

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1160
> -void FrameLoaderClientImpl::didRunInsecureContent(SecurityOrigin* origin)
> +void FrameLoaderClientImpl::didRunInsecureContent(SecurityOrigin* origin, const KURL& targetURL)
>  {
>      if (m_webFrame->client())
> -        m_webFrame->client()->didRunInsecureContent(m_webFrame, WebSecurityOrigin(origin));
> +        m_webFrame->client()->didRunInsecureContent(m_webFrame, WebSecurityOrigin(origin), targetURL);
>  }

Its kind of lame that the parameter changes name.  Seems like it should be insecureURL everywhere...
Comment 19 Darin Fisher (:fishd, Google) 2011-02-01 14:15:17 PST
(In reply to comment #18)
> Its kind of lame that the parameter changes name.  Seems like it should be insecureURL everywhere...

I like using that name everywhere.  However, I'll just point out that it is OK for terms used in the WebKit API to differ from terms used in WebCore.  It is nice for them to be the same, but WebCore can and does change out from underneath the WebKit API.  So, terms that were once consistent may end up becoming inconsistent over time.  What is more important is that the WebKit API stand on its own and make sense without the context of whatever WebCore may be doing.
Comment 20 Adam Langley 2011-02-03 11:55:20 PST
Created attachment 81095 [details]
updated targetURL to insecureURL
Comment 21 Adam Barth 2011-02-03 12:22:48 PST
Comment on attachment 81095 [details]
updated targetURL to insecureURL

Thanks!
Comment 22 WebKit Commit Bot 2011-02-03 21:30:23 PST
Comment on attachment 81095 [details]
updated targetURL to insecureURL

Clearing flags on attachment: 81095

Committed r77602: <http://trac.webkit.org/changeset/77602>
Comment 23 WebKit Commit Bot 2011-02-03 21:30:32 PST
All reviewed patches have been landed.  Closing bug.