Address the FIXME comments added for the build for for Bug 146384: Committed r186036: <http://trac.webkit.org/changeset/186036>
Created attachment 255717 [details] Patch v1
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.
(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?
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.
(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.
Created attachment 267825 [details] Patch v2
(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().
Created attachment 267885 [details] Patch v3 (checking EFL/GTK builds)
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.
Created attachment 267886 [details] Patch v4
Created attachment 267891 [details] Patch v5
Created attachment 267892 [details] Patch v6
Created attachment 267896 [details] Patch v7
Created attachment 267897 [details] Patch v8
Created attachment 267898 [details] Patch v9
Created attachment 267899 [details] Patch v10
Comment on attachment 267899 [details] Patch v10 Clearing flags on attachment: 267899 Committed r194419: <http://trac.webkit.org/changeset/194419>
All reviewed patches have been landed. Closing bug.
(In reply to comment #18) > All reviewed patches have been landed. Closing bug. It broke the WinCairo (curl) build.
(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 :)
(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.
(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.