Bug 184755 – Refactor NetworkResourceLoader to check for m_networkLoadChecker presence before using it

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[youenn fablet]

This will allow to have code ready when using NetworkLoadChecker for async loads

[youenn fablet]

Created attachment 338279 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.h:151:  The parameter name "request" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


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

[youenn fablet]

Created attachment 338285 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.h:151:  The parameter name "request" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


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

[EWS Watchlist]

Comment on attachment 338285 [details]
Patch

Attachment 338285 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7364943

New failing tests:
http/tests/xmlhttprequest/redirect-cross-origin-tripmine.html

[EWS Watchlist]

Created attachment 338289 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 338285 [details]
Patch

Attachment 338285 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7365103

New failing tests:
http/tests/xmlhttprequest/redirect-cross-origin-tripmine.html

[EWS Watchlist]

Created attachment 338290 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[youenn fablet]

Created attachment 338311 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.h:152:  The parameter name "request" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 9 files


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

[Chris Dumez]

Comment on attachment 338311 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:347
> +                if (!protectedThis->m_isStopped)

Do we really need this new flag? Cannot we rely on m_networkLoad being null?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:488
> +                if (!result.error().isCancellation())

I do not quite understand this new check. We used to run this code no matter the error type.

Also, nobody calls continueWillSendRequest() now, why is it OK?

[youenn fablet]

Comment on attachment 338311 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:347
>> +                if (!protectedThis->m_isStopped)
> 
> Do we really need this new flag? Cannot we rely on m_networkLoad being null?

OK, will try it.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:488
>> +                if (!result.error().isCancellation())
> 
> I do not quite understand this new check. We used to run this code no matter the error type.
> 
> Also, nobody calls continueWillSendRequest() now, why is it OK?

This check is already used for m_networkLoadChecker->check(). It is to make sure 'this' is still valid.
It might be possible to write an API test involving sync XHR and closing the tab but this would require an API test with a server and it will be covered when extending m_networkLoadChecker tests.

As of not calling continueWillSendRequest(), we cannot do that since it would call didFail with a cancel error and we want to send the real error.
didFailLoading is cleaning everything so is also clearing m_networkLoad.
FWIW, m_networkLoad needs already to handle the case of waiting for continueWillSendRequest but being actually destroyed for async loads for instance.

[youenn fablet]

Created attachment 338449 [details]
Removing m_isStopped

[EWS Watchlist]

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


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.h:152:  The parameter name "request" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 9 files


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

[WebKit Commit Bot]

Comment on attachment 338449 [details]
Removing m_isStopped

Clearing flags on attachment: 338449

Committed r230857: <https://trac.webkit.org/changeset/230857>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39610615>