Bug 195497 - Take UnboundedNetworking assertion when a file upload is in progress
Summary: Take UnboundedNetworking assertion when a file upload is in progress
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: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-08 17:06 PST by Brady Eidson
Modified: 2019-03-13 10:59 PDT (History)
4 users (show)

See Also:


Attachments
Patch (29.08 KB, patch)
2019-03-12 14:43 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (29.08 KB, patch)
2019-03-13 09:41 PDT, Brady Eidson
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2019-03-08 17:06:23 PST
Take UnboundedNetworking assertion when a file upload is in progress
Comment 1 Brady Eidson 2019-03-12 14:43:47 PDT
Created attachment 364448 [details]
Patch
Comment 2 Geoffrey Garen 2019-03-12 15:29:42 PDT
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.
Comment 3 Brady Eidson 2019-03-12 16:19:54 PDT
(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!

:)
Comment 4 Brady Eidson 2019-03-13 09:41:02 PDT
Created attachment 364541 [details]
Patch
Comment 5 Geoffrey Garen 2019-03-13 10:32:09 PDT
Comment on attachment 364541 [details]
Patch

Not sure if I should review, but r=me :P
Comment 6 Chris Dumez 2019-03-13 10:36:35 PDT
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 7 Chris Dumez 2019-03-13 10:39:08 PDT
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 8 Chris Dumez 2019-03-13 10:41:51 PDT
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.
Comment 9 Brady Eidson 2019-03-13 10:48:03 PDT
(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.
Comment 10 Chris Dumez 2019-03-13 10:56:30 PDT
(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.
Comment 11 Brady Eidson 2019-03-13 10:58:03 PDT
Committed r242891: <https://trac.webkit.org/changeset/242891>
Comment 12 Radar WebKit Bug Importer 2019-03-13 10:59:18 PDT
<rdar://problem/48854017>