We should give them a void return value instead. Code: dom/fetch/FetchDriver.cpp
I'm interested in fixing this bug. Can you please assign it to me?
Great! Let me or jdm know if you need help.
Assignee: nobody → stefandye
Status: NEW → ASSIGNED
I'm confused as to what direction to take with this bug fix. I've messaged khuey with this already to apologies for the double post. The fix requested was to have SucceedWIthResponse() and FailWithNetworkError() return nothing, At first glance I though this meant the signatures should be have void return types in dom/fetch/FetchDriver.cpp. However some functions like FetchDriver::BasicFetch() have an nsresult return type and are returning the output of one of these functions (SucceedWIthResponse for this function) in the function definition. What I can think of right away are these two solutions: Option 1 : The most obvious thing to do would be to make the caller's return type void, but that's likely to propagate through many other functions so I have a feeling that's not the right solution. Option 2: Return NS_OK from the caller functions that require an nsresult. I'm unsure if that's a good idea, since the goal of the fix was to eliminate the same issue in SucceedWithResponse() and FaulWithNetworkError(). . However, after reading the fetch specification at https://fetch.spec.whatwg.org/, it seems that return a network error which is defined as a 0 response, empty bytes sequence . " A network error is a response whose status is always 0, status message is always the empty byte sequence, header list is always empty, body is always null, and cache state is always "none"". It seems this is handled by line 714 in FetchDriver.cpp : nsRefPtr<InternalResponse> error = InternalResponse::NetworkError(); This would seem to imply that option 2 may work. Is this true? Or if not can you please explain? Any guidance and clarification would be of great help. Thanks, Stefan
The solution here is to move the `return NS_OK` to the callers. This should not cause the same situation because they contain instance of returning non-NS_OK values (the NS_ENSURE_SUCCESS macros contain a hidden return in the error case).
Created attachment 8668760 [details] [diff] [review] FetchDriver.h Message for jdm. Please review. Uploaded FetchDriver.h . (1 of 2 files)
Created attachment 8668761 [details] [diff] [review] FetchDriver.cpp Uploaded for review by :jdm . Please review FetchDriver.cpp patch (2 of 2)
For future reference, I recommend setting the `review` flag to `?` and adding `:jdm`. That will specifically alert me that there are changes awaiting my review. Also, combining the changes into a single diff file is preferred, if they are part of the same set of changes.
Comment on attachment 8668761 [details] [diff] [review] FetchDriver.cpp Review of attachment 8668761 [details] [diff] [review]: ----------------------------------------------------------------- This is generally moving in the right direction, but we need to ensure that we continue to return from the methods at the same time the old code did, otherwise we will end up changing the behaviour of the callers of SucceedWithResponse/FailWithNetwork error. Does that make sense? A good way to verify that these changes are working correctly is to run the appropriate tests: `./mach mochitest dom/tests/mochitest/fetch` ::: dom/fetch/FetchDriver.cpp @@ +208,4 @@ > } > > MOZ_ASSERT_UNREACHABLE("Unexpected main fetch operation!"); > + FailWithNetworkError(); nit: remove the extra space before Fail. @@ +648,5 @@ > MOZ_ASSERT(mResponse); > MOZ_ASSERT(!mResponse->IsError()); > > + SucceedWithResponse(); > +return NS_OK; nit: indent this to match the previous line, please. @@ +699,4 @@ > // Release the ref. > } > > +void //nsresult There's no need for comments like this, since the whole premise of diff files is that they show the before and after changes. @@ +707,5 @@ > mObserver->OnResponseEnd(); > mObserver = nullptr; > } > +// removed NS_OK as this is a now void function, see bug 1204520. > +//since SuccedWithResponse handles the observers internally, NS_OK is not needed. These comments don't really provide any additional information over looking in the file history, so let's remove them. @@ +722,5 @@ > mObserver->OnResponseEnd(); > mObserver = nullptr; > } > +// removed NS_OK as this is a now void function, see bug 1204520 > +// in the case of a failure, the Network error is communicated via an internal response object, therefore the NS_OK return result is not needed. Same here. @@ +1118,4 @@ > } > > } // namespace dom > +} Please revert this change.
Attachment #8668761 - Flags: feedback+
Hey jdm, Thanks for providing feedback. I've noted the above comments and am making the changes. Just wish to clarify one point. "but we need to ensure that we continue to return from the methods at the same time the old code did, otherwise we will end up changing the behaviour of the callers of SucceedWithResponse/FailWithNetwork error." Is this a rule of thumb? Just curious because I ran the `./mach mochitest dom/tests/mochitest/fetch` and got zero failures with the previous implementation. But from your statement I'm inferring that other functions that rely on the fetch driver implementation and not covered by this suite of tests may break because of this change? Please elaborate if I'm incorrect.
I wouldn't call it a rule of thumb, more like a rule of refactoring :) I admit that I'm extremely surprised that the tests didn't display any failures! Perhaps you're using a release build, not a debug one? https://developer.mozilla.org/en-US/docs/Simple_Firefox_build#Build_configuration_%28optional%29 shows how to make that switch.
I'd like to help what should i do?
Hi Dalak! It's been a month since Stefan replied, so you could import the patch attached to this bug and address the review comments from comment 8.
Hey jdm, I ran sudo ./mach mochitest ./dom/fetch/ got some unexpected fails. Where are the logs stored? I just want to be able to trace where those failures are coming from.
Sorry for the delay in replying; I was at a conference all last week that took up all my attention. There are no stored logs by default. You would need to run the mach command with >mochitest.log for that. I'll also point out that it should never be necessary to use sudo with mach.
Created attachment 8698327 [details] [diff] [review] FetchDriverPatch Hey jdm, Uploading my patch for review. 39 tests passed. 8 tests failed. I also ran the same test after building the source cleanly and got the same results. So it seems my patch added no new bugs
Attachment #8698327 - Attachment mime type: text/x-patch → text/plain
Attachment #8698327 - Attachment is patch: true
Comment on attachment 8698327 [details] [diff] [review] FetchDriverPatch Review of attachment 8698327 [details] [diff] [review]: ----------------------------------------------------------------- Hi Stefan! Sorry for the long wait; this came at the tail end of week-long conference and then there were holidays, etc. I find it strange that you see test failures before making changes, since the tests are all supposed to be passing. Are you sure you made a build that didn't include any changes before running the tests? Regardless, we're going to need to ensure that every piece of code that used to have `return FailWithNetworkError` or `return SucceedWithResponse` continues to return from the method after these changes. There are a lot of unintentional control flow changes in the current patch. ::: dom/fetch/FetchDriver.cpp @@ +252,4 @@ > > response->SetBody(body); > BeginResponse(response); > + SucceedWithResponse(); This is the bit that really surprises me when you say that you don't get any different test results, given that we used to return from this method, and now fall through to a failure case. We're definitely going to need to change this to continue returning here.
Perhaps running `./mach mochitest-plain dom/workers/test/serviceworkers/` or `./mach web-platform-tests testing/web-platform/mozilla/tests/service-workers/service-worker` would expose some errors with these changes...
Hey Josh, Apologies for being M.I.A. I had to actually get a new computer! Was finally able to re-checkout the mozilla source and redo the patch. I am no longer seeing the SucceedWithResponse function in FetchDriver.cpp. Running ./mach build now yields no errors. And the fix seems to work. Uploading FetchTests.txt with test results. FetchDriver.cpp and FetchDriver.h patch.
Created attachment 8714196 [details] [diff] [review] FetchDriver header and cpp changes, second uploadbnbn
Created attachment 8714197 [details] FetchTests.txt results of test run
Comment on attachment 8714196 [details] [diff] [review] FetchDriver header and cpp changes, second uploadbnbn Review of attachment 8714196 [details] [diff] [review]: ----------------------------------------------------------------- Huh, this code has changed significantly since you originally started looking at this, apparently :)
Attachment #8714196 - Flags: review?(josh) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/122ae2d1cf825ec75509233c5cc26db6df2b2b99 Bug 1204520 - Remove unused return value from FetchDriver::FailWithNetworkError. r=jdm
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.