Bug 52384 – Plumb mixed script URL to FrameLoaderClient

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Langley]

Plumb mixed script URL to FrameLoaderClient

[Adam Langley]

Created attachment 78833 [details]
Patch

[Early Warning System Bot]

Attachment 78833 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7507019

[Adam Langley]

Created attachment 78845 [details]
Patch

[Adam Langley]

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.

[Adam Barth]

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
> [phone]  Adam Langley  <[e-mail]>

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.

[Alexey Proskuryakov]

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.

[Adam Langley]

(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.

[Adam Langley]

Created attachment 79496 [details]
Patch

[Build Bot]

Attachment 79496 [details] did not build on win:
Build output: http://queues.webkit.org/results/7496227

[Early Warning System Bot]

Attachment 79496 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7539231

[Adam Langley]

Created attachment 79515 [details]
Patch

[Eric Seidel (no email)]

Attachment 79515 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7633082

[Adam Langley]

(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.

[Adam Barth]

Looks good to me, but we'll need fishd to give the official review because this changes the Chromium WebKit API.

[Eric Seidel (no email)]

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.

[Darin Fisher (:fishd, Google)]

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.

[Adam Langley]

Created attachment 80698 [details]
Patch

[Adam Barth]

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...

[Darin Fisher (:fishd, Google)]

(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.

[Adam Langley]

Created attachment 81095 [details]
updated targetURL to insecureURL

[Adam Barth]

Comment on attachment 81095 [details]
updated targetURL to insecureURL

Thanks!

[WebKit Commit Bot]

Comment on attachment 81095 [details]
updated targetURL to insecureURL

Clearing flags on attachment: 81095

Committed r77602: <http://trac.webkit.org/changeset/77602>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.