Bug 184755 - Refactor NetworkResourceLoader to check for m_networkLoadChecker presence before using it
Summary: Refactor NetworkResourceLoader to check for m_networkLoadChecker presence bef...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-18 14:14 PDT by youenn fablet
Modified: 2018-04-20 15:05 PDT (History)
5 users (show)

See Also:


Attachments
Patch (14.50 KB, patch)
2018-04-18 16:53 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (14.51 KB, patch)
2018-04-18 17:13 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.50 MB, application/zip)
2018-04-18 17:54 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews200 for win-future (12.65 MB, application/zip)
2018-04-18 18:24 PDT, EWS Watchlist
no flags Details
Patch (23.21 KB, patch)
2018-04-18 22:46 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Removing m_isStopped (22.76 KB, patch)
2018-04-20 12:04 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-04-18 14:14:16 PDT
This will allow to have code ready when using NetworkLoadChecker for async loads
Comment 1 youenn fablet 2018-04-18 16:53:51 PDT
Created attachment 338279 [details]
Patch
Comment 2 EWS Watchlist 2018-04-18 16:56:28 PDT
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.
Comment 3 youenn fablet 2018-04-18 17:13:04 PDT
Created attachment 338285 [details]
Patch
Comment 4 EWS Watchlist 2018-04-18 17:15:55 PDT
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.
Comment 5 EWS Watchlist 2018-04-18 17:54:43 PDT
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
Comment 6 EWS Watchlist 2018-04-18 17:54:44 PDT
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
Comment 7 EWS Watchlist 2018-04-18 18:24:14 PDT
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
Comment 8 EWS Watchlist 2018-04-18 18:24:25 PDT
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
Comment 9 youenn fablet 2018-04-18 22:46:23 PDT
Created attachment 338311 [details]
Patch
Comment 10 EWS Watchlist 2018-04-18 22:49:04 PDT
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.
Comment 11 Chris Dumez 2018-04-20 09:32:52 PDT
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?
Comment 12 youenn fablet 2018-04-20 12:03:50 PDT
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.
Comment 13 youenn fablet 2018-04-20 12:04:14 PDT
Created attachment 338449 [details]
Removing m_isStopped
Comment 14 EWS Watchlist 2018-04-20 12:09:58 PDT
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.
Comment 15 WebKit Commit Bot 2018-04-20 15:03:44 PDT
Comment on attachment 338449 [details]
Removing m_isStopped

Clearing flags on attachment: 338449

Committed r230857: <https://trac.webkit.org/changeset/230857>
Comment 16 WebKit Commit Bot 2018-04-20 15:03:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2018-04-20 15:05:18 PDT
<rdar://problem/39610615>