Bug 152489 – Allow JavaScript to iterate over plugins for local SecurityOrigins

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

This bug refines Bug 151783 slightly based on Darin's suggestion: "I’d really prefer that this security check be done with SecurityOrigin function rather than a direct call to isLocalFile.".

This patch revises the code slightly to do just that.

[Brent Fulgham]

Created attachment 267761 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 267761 [details]
Patch

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

> Source/WebCore/page/Page.cpp:532
> +    DocumentLoader* documentLoader = mainFrame().loader().documentLoader();
> +    if (!documentLoader)
> +        return false;

There wasn't a null check for documentLoader before, but there is one now. Can a regression test be added for this change?

> Source/WebCore/page/Page.cpp:534
> +    RefPtr<SecurityOrigin> origin(SecurityOrigin::create(documentLoader->url()));

SecurityOrigin::create returns a Ref, so please don't make a RefPtr out of it. I think that this is the right way to write this:

Ref<SecurityOrigin> origin = SecurityOrigin::create(documentLoader->url());

But more importantly, I think that we should use the actual security origin for the document, not a one reconstructed from its URL. I don't know of an observable difference, but calling securityOrigin() on the document feels like a better thing to do.

[Brent Fulgham]

(In reply to comment #2)
> Comment on attachment 267761 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267761&action=review
> 
> > Source/WebCore/page/Page.cpp:532
> > +    DocumentLoader* documentLoader = mainFrame().loader().documentLoader();
> > +    if (!documentLoader)
> > +        return false;
> 
> There wasn't a null check for documentLoader before, but there is one now.
> Can a regression test be added for this change?

After looking at some of the other uses of documentLoader here (and elsewhere), I noticed that many of them are doing null checks, so I added one here. I'm not sure how to create a test that hits this code with a nullptr documentLoader.

> > Source/WebCore/page/Page.cpp:534
> > +    RefPtr<SecurityOrigin> origin(SecurityOrigin::create(documentLoader->url()));
> 
> SecurityOrigin::create returns a Ref, so please don't make a RefPtr out of
> it. I think that this is the right way to write this:
> 
> Ref<SecurityOrigin> origin = SecurityOrigin::create(documentLoader->url());

Done.

> But more importantly, I think that we should use the actual security origin
> for the document, not a one reconstructed from its URL. I don't know of an
> observable difference, but calling securityOrigin() on the document feels
> like a better thing to do.

I can certainly do that. However, it looks like securityOrigin() is not guaranteed to return a value, so I think we should use the securityOrigin if it exists, otherwise we have to generate one from the URL.

[Brent Fulgham]

Created attachment 267790 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 267790 [details]
Patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:197
> +    } else if (DocumentLoader* documentLoader = page->mainFrame().loader().documentLoader()) {

Do we use this idiom anywhere else? I would expect that if document->securityOrigin() returns null, then there isn't a security origin for us to use, so we should not fall back to the URL.

[Brent Fulgham]

(In reply to comment #5)
> Comment on attachment 267790 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267790&action=review
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:197
> > +    } else if (DocumentLoader* documentLoader = page->mainFrame().loader().documentLoader()) {
> 
> Do we use this idiom anywhere else? I would expect that if
> document->securityOrigin() returns null, then there isn't a security origin
> for us to use, so we should not fall back to the URL.

I have to admit I cannot find a single place where we don't just trust the return value of document()->securityOrigin(). It must be 'guaranteed' to exist.

I'll remove that fallback case.

[Brent Fulgham]

Created attachment 267791 [details]
Patch

[Brent Fulgham]

Committed r194367: <http://trac.webkit.org/changeset/194367>