1204520 - FetchDriver::SucceedWithResponse/FailWithNetworkError always return NS_OK

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Josh Matthews [:jdm]]

We should give them a void return value instead.

Code: dom/fetch/FetchDriver.cpp

Comment 1

[Stefan Dye]

I'm interested in fixing this bug. Can you please assign it to me?

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Great!  Let me or jdm know if you need help.

Comment 3

[Stefan Dye]

 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

Comment 4

[Josh Matthews [:jdm]]

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

Comment 5

[Stefan Dye]

Attachment: FetchDriver.h

Message for jdm. Please review. Uploaded FetchDriver.h . (1 of 2 files)

Comment 6

[Stefan Dye]

Attachment: FetchDriver.cpp

Uploaded for review by :jdm . Please review FetchDriver.cpp patch (2 of 2)

Comment 7

[Josh Matthews [:jdm]]

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 8

[Josh Matthews [:jdm]]

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.

Comment 9

[Stefan Dye]

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.

Comment 10

[Josh Matthews [:jdm]]

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.

Comment 11

[dalak.bartek]

I'd like to help what should i do?

Comment 12

[Josh Matthews [:jdm]]

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.

Comment 13

[Stefan Dye]

Attachment: dom mochitest run

Comment 14

[Stefan Dye]

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.

Comment 15

[Josh Matthews [:jdm]]

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.

Comment 16

[Stefan Dye]

Attachment: 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

Comment 17

[Josh Matthews [:jdm]]

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.

Comment 18

[Josh Matthews [:jdm]]

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

Comment 19

[Stefan Dye]

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.

Comment 20

[Stefan Dye]

Attachment: FetchDriver header and cpp changes, second uploadbnbn

Comment 21

[Stefan Dye]

Attachment: FetchTests.txt results of test run

Comment 22

[Josh Matthews [:jdm]]

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 :)

Comment 23

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/122ae2d1cf825ec75509233c5cc26db6df2b2b99
Bug 1204520 - Remove unused return value from FetchDriver::FailWithNetworkError. r=jdm

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/122ae2d1cf82