Bug 146391 – ResourceError should store failingURL as URL instead of String to avoid reparsing and to address FIXME comments in ResourceErrorCF.cpp and ResourceErrorMac.mm

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[David Kilzer (:ddkilzer)]

Address the FIXME comments added for the build for for Bug 146384:
Committed r186036: <http://trac.webkit.org/changeset/186036>

[David Kilzer (:ddkilzer)]

Created attachment 255717 [details]
Patch v1

[Alexey Proskuryakov]

Comment on attachment 255717 [details]
Patch v1

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

> Source/WebCore/platform/network/cf/ResourceErrorCF.cpp:157
> +            if (RetainPtr<CFURLRef> url = URL(URL(), m_failingURL).createCFURL())

Can this be tested?

As a general principle, I think that we shouldn't expose invalid URLs in API. The fact that the URL is invalid gets lost in conversion, and it's too easy for the client to make incorrect decisions based on the URL string. I can't find it now, but we even had security bugs caused by handling invalid URLs.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> Comment on attachment 255717 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255717&action=review
> 
> > Source/WebCore/platform/network/cf/ResourceErrorCF.cpp:157
> > +            if (RetainPtr<CFURLRef> url = URL(URL(), m_failingURL).createCFURL())
> 
> Can this be tested?

It's not known how to reproduce the crash.  Do you have ideas about how to reproduce it based on the crashing stack?  Maybe setting the source of an iframe with an invalid URL?

[Darin Adler]

Comment on attachment 255717 [details]
Patch v1

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

Before we make this change, I’d like to understand more about where the string is coming from. In particular, did this already come out of a WebCore::URL, and if so, can we avoid the round trip through a string?

>>> Source/WebCore/platform/network/cf/ResourceErrorCF.cpp:157
>>> +            if (RetainPtr<CFURLRef> url = URL(URL(), m_failingURL).createCFURL())
>> 
>> Can this be tested?
>> 
>> As a general principle, I think that we shouldn't expose invalid URLs in API. The fact that the URL is invalid gets lost in conversion, and it's too easy for the client to make incorrect decisions based on the URL string. I can't find it now, but we even had security bugs caused by handling invalid URLs.
> 
> It's not known how to reproduce the crash.  Do you have ideas about how to reproduce it based on the crashing stack?  Maybe setting the source of an iframe with an invalid URL?

I think you have an interesting point, Alexey, about invalid URLs. But please keep in mind that we are exposing that same URL above in failingURLStringKey; I am not certain that an NSURL is an additional risk.

> Source/WebCore/platform/network/mac/ResourceErrorMac.mm:186
> +        if (RetainPtr<NSURL> cocoaURL = (NSURL *)URL(URL(), resourceError.failingURL()))

No reason to use RetainPtr here instead of just NSURL *. The NSURL will be autoreleased.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> Comment on attachment 255717 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255717&action=review
> 
> Before we make this change, I’d like to understand more about where the
> string is coming from. In particular, did this already come out of a
> WebCore::URL, and if so, can we avoid the round trip through a string?

The CFErrorRef and NSError objects come from CFNetwork callbacks, so I don't think there are any round trip savings to be had here.

> >>> Source/WebCore/platform/network/cf/ResourceErrorCF.cpp:157
> >>> +            if (RetainPtr<CFURLRef> url = URL(URL(), m_failingURL).createCFURL())
> >> 
> >> Can this be tested?
> >> 
> >> As a general principle, I think that we shouldn't expose invalid URLs in API. The fact that the URL is invalid gets lost in conversion, and it's too easy for the client to make incorrect decisions based on the URL string. I can't find it now, but we even had security bugs caused by handling invalid URLs.
> > 
> > It's not known how to reproduce the crash.  Do you have ideas about how to reproduce it based on the crashing stack?  Maybe setting the source of an iframe with an invalid URL?
> 
> I think you have an interesting point, Alexey, about invalid URLs. But
> please keep in mind that we are exposing that same URL above in
> failingURLStringKey; I am not certain that an NSURL is an additional risk.
> 
> > Source/WebCore/platform/network/mac/ResourceErrorMac.mm:186
> > +        if (RetainPtr<NSURL> cocoaURL = (NSURL *)URL(URL(), resourceError.failingURL()))
> 
> No reason to use RetainPtr here instead of just NSURL *. The NSURL will be
> autoreleased.

I thought we preferred explicit retain/release instead of putting objects into autoreleasepools (generally speaking), but removing RetainPtr<> will make the conversion to ARC easier when that happens, and it's only a single object being put in the pool, so I'll change it.

[David Kilzer (:ddkilzer)]

Created attachment 267825 [details]
Patch v2

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 255717 [details]
> > Patch v1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=255717&action=review
> > 
> > Before we make this change, I’d like to understand more about where the
> > string is coming from. In particular, did this already come out of a
> > WebCore::URL, and if so, can we avoid the round trip through a string?
> 
> The CFErrorRef and NSError objects come from CFNetwork callbacks, so I don't
> think there are any round trip savings to be had here.

Oops, I was looking in the wrong direction!

Looking at the ResourceError() constructors that use the multi-argument constructor, I think nearly all of them are converting a URL() to a String(), so we could save a round trip here.

I will try changing the failingURL argument of ResourceError() from String() to URL().

[David Kilzer (:ddkilzer)]

Created attachment 267885 [details]
Patch v3 (checking EFL/GTK builds)

[WebKit Commit Bot]

Attachment 267885 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/gtk/ErrorsGtk.cpp:33:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/gtk/ErrorsGtk.cpp:39:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/gtk/ErrorsGtk.cpp:50:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/gtk/ErrorsGtk.cpp:56:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/gtk/ErrorsGtk.cpp:62:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/gtk/ErrorsGtk.cpp:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/gtk/ErrorsGtk.cpp:74:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/gtk/ErrorsGtk.cpp:86:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/gtk/ErrorsGtk.cpp:92:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 9 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Created attachment 267886 [details]
Patch v4

[David Kilzer (:ddkilzer)]

Created attachment 267891 [details]
Patch v5

[David Kilzer (:ddkilzer)]

Created attachment 267892 [details]
Patch v6

[David Kilzer (:ddkilzer)]

Created attachment 267896 [details]
Patch v7

[David Kilzer (:ddkilzer)]

Created attachment 267897 [details]
Patch v8

[David Kilzer (:ddkilzer)]

Created attachment 267898 [details]
Patch v9

[David Kilzer (:ddkilzer)]

Created attachment 267899 [details]
Patch v10

[WebKit Commit Bot]

Comment on attachment 267899 [details]
Patch v10

Clearing flags on attachment: 267899

Committed r194419: <http://trac.webkit.org/changeset/194419>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #18)
> All reviewed patches have been landed.  Closing bug.

It broke the WinCairo (curl) build.

[David Kilzer (:ddkilzer)]

(In reply to comment #19)
> (In reply to comment #18)
> > All reviewed patches have been landed.  Closing bug.
> 
> It broke the WinCairo (curl) build.

That build needs to be on the dashboard.

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > All reviewed patches have been landed.  Closing bug.
> > 
> > It broke the WinCairo (curl) build.
> 
> That build needs to be on the dashboard.

Yes, that would be very nice :)

[Csaba Osztrogonác]

(In reply to comment #19)
> (In reply to comment #18)
> > All reviewed patches have been landed.  Closing bug.
> 
> It broke the WinCairo (curl) build.

Fixed in bug152563. Thanks.

[Darin Adler]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 255717 [details]
> > Patch v1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=255717&action=review
> > 
> > > Source/WebCore/platform/network/mac/ResourceErrorMac.mm:186
> > > +        if (RetainPtr<NSURL> cocoaURL = (NSURL *)URL(URL(), resourceError.failingURL()))
> > 
> > No reason to use RetainPtr here instead of just NSURL *. The NSURL will be
> > autoreleased.
> 
> I thought we preferred explicit retain/release instead of putting objects
> into autoreleasepools (generally speaking), but removing RetainPtr<> will
> make the conversion to ARC easier when that happens, and it's only a single
> object being put in the pool, so I'll change it.

While this code no longer exists, I wanted to clear up a misunderstanding here. The local decision here had nothing to do with conversion to ARC.

The function that converts a URL to an NSURL puts the URL into the autorelease pool. Using a RetainPtr doesn't prevent it from doing that, instead it retains and releases an additional time, unnecessarily. There's no choice between autorelease and retain/release in this code, just a choice between doing an extra retain/release or not doing it.

If we wanted to use retain/release instead of an autorelease pool we would have to change the function that converts a URL into an NSURL * so that it doesn't autorelease.