Summary: | Take UnboundedNetworking assertion when a file upload is in progress | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||
Component: | WebKit Misc. | Assignee: | Brady Eidson <beidson> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aestes, cdumez, ggaren, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Brady Eidson
2019-03-08 17:06:23 PST
Created attachment 364448 [details]
Patch
Comment on attachment 364448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364448&action=review r=me, some comments below, also please update to trunk > Source/WebKit/ChangeLog:8 > + This patch implememts whole bunch of bookkeeping in both the Networking and UI processes. implememts => implements a > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:808 > +void NetworkConnectionToWebProcess::setProcessIdentifier(ProcessIdentifier processIdentifier) setProcessIdentifier => setWebProcessIdentifier processIdentifier => webProcessIdentifier > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:813 > +void NetworkConnectionToWebProcess::connectionHasUploads() connectionHasUploads => setConnectionHasUploads > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:820 > +void NetworkConnectionToWebProcess::connectionNoLongerHasUploads() connectionNoLongerHasUploads => clearConnectionHasUploads > Source/WebKit/NetworkProcess/NetworkResourceLoadMap.cpp:39 > + if (m_loadersWithUploads.size() == 1) > + m_connectionToWebProcess.connectionHasUploads(); Nit: I think it's slightly clearer to check for zero before inserting than it is to check for 1 (or more) after inserting. One reason is that it matches the check in remove. Another reason is that zero is always zero, but non-zero is not always 1. > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1200 > +void NetworkProcessProxy::releaseUploadAssertion() releaseUploadAssertion => clearUploadAssertion > Source/WebKit/UIProcess/WebProcessPool.cpp:2502 > + if (m_processesWithUploads.size() == 1) { Same comment about 1. (In reply to Geoffrey Garen from comment #2) > Comment on attachment 364448 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=364448&action=review > > r=me, some comments below, also please update to trunk I was planning on *NOT* updating to trunk, though! :) Created attachment 364541 [details]
Patch
Comment on attachment 364541 [details]
Patch
Not sure if I should review, but r=me :P
Comment on attachment 364541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364541&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:64 > + std::unique_ptr<ProcessAssertion> m_networkAssertion; Where is this initialized? Comment on attachment 364541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364541&action=review > Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:268 > + std::unique_ptr<ProcessAssertion> m_uploadAssertion; If you take a ProcessAssertion and not a ProcessAndUIAssertion, then what prevents the UIProcess from getting suspended? If the UIProcess gets suspended, how can this unbounded assertion get released? Comment on attachment 364541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364541&action=review > Source/WebKit/UIProcess/WebProcessPool.h:776 > + std::unique_ptr<ProcessAssertion> m_uiProcessUploadAssertion; Currently seems like a misnomer no? AFAICT, it does not prevent UIProcess suspension at the moment. (In reply to Chris Dumez from comment #7) > Comment on attachment 364541 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=364541&action=review > > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:268 > > + std::unique_ptr<ProcessAssertion> m_uploadAssertion; > > If you take a ProcessAssertion and not a ProcessAndUIAssertion, then what > prevents the UIProcess from getting suspended? > If the UIProcess gets suspended, how can this unbounded assertion get > released? Let's talk in person, but basically the names you think are important are not. (In reply to Brady Eidson from comment #9) > (In reply to Chris Dumez from comment #7) > > Comment on attachment 364541 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=364541&action=review > > > > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:268 > > > + std::unique_ptr<ProcessAssertion> m_uploadAssertion; > > > > If you take a ProcessAssertion and not a ProcessAndUIAssertion, then what > > prevents the UIProcess from getting suspended? > > If the UIProcess gets suspended, how can this unbounded assertion get > > released? > > Let's talk in person, but basically the names you think are important are > not. Ok, looks good to me now besides the unused data member. Committed r242891: <https://trac.webkit.org/changeset/242891> |