FetchDriver::SucceedWithResponse/FailWithNetworkError always return NS_OK

RESOLVED FIXED in Firefox 47

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jdm, Assigned: Stefan Dye, Mentored)

Tracking

Trunk
mozilla47
Points:
---

Firefox Tracking Flags

(firefox43 affected, firefox47 fixed)

Details

(Whiteboard: [lang=c++][good first bug])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
We should give them a void return value instead.

Code: dom/fetch/FetchDriver.cpp
(Assignee)

Comment 1

3 years ago
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
(Assignee)

Comment 3

2 years ago
 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
Flags: needinfo?(josh)
(Reporter)

Comment 4

2 years ago
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).
Flags: needinfo?(josh)
(Assignee)

Comment 5

2 years ago
Created attachment 8668760 [details] [diff] [review]
FetchDriver.h

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

Comment 6

2 years ago
Created attachment 8668761 [details] [diff] [review]
FetchDriver.cpp

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

Comment 7

2 years ago
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.
(Reporter)

Comment 8

2 years ago
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+
(Assignee)

Comment 9

2 years ago
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.
(Reporter)

Comment 10

2 years ago
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

2 years ago
I'd like to help what should i do?
(Reporter)

Comment 12

2 years ago
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.
(Assignee)

Comment 13

2 years ago
Created attachment 8696600 [details]
dom mochitest run
(Assignee)

Comment 14

2 years ago
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.
(Reporter)

Comment 15

2 years ago
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.
(Assignee)

Comment 16

2 years ago
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 #8668760 - Attachment is obsolete: true
Attachment #8668761 - Attachment is obsolete: true
Attachment #8698327 - Flags: review?(josh)
(Reporter)

Updated

2 years ago
Attachment #8698327 - Attachment mime type: text/x-patch → text/plain
(Reporter)

Updated

2 years ago
Attachment #8698327 - Attachment is patch: true
(Reporter)

Comment 17

2 years ago
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.
Attachment #8698327 - Flags: review?(josh)
(Reporter)

Comment 18

2 years ago
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...
(Assignee)

Comment 19

2 years ago
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.
(Assignee)

Comment 20

2 years ago
Created attachment 8714196 [details] [diff] [review]
FetchDriver header and cpp changes, second uploadbnbn
Attachment #8698327 - Attachment is obsolete: true
Attachment #8714196 - Flags: review?(josh)
(Assignee)

Comment 21

2 years ago
Created attachment 8714197 [details]
FetchTests.txt results of test run
Attachment #8696600 - Attachment is obsolete: true
Attachment #8714197 - Flags: review?(josh)
(Reporter)

Updated

2 years ago
Attachment #8714196 - Attachment is patch: true
Attachment #8714196 - Attachment mime type: text/x-patch → text/plain
(Reporter)

Comment 22

2 years ago
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+
(Reporter)

Updated

2 years ago
Attachment #8714197 - Flags: review?(josh)
(Reporter)

Comment 23

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

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/122ae2d1cf82
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.