Bug 146439

Summary: Web page doesn't update its loading state when web process becomes suspended if there are pending network requests (XHR).
Product: WebKit Reporter: Yongjun Zhang <yongjun_zhang>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, barraclough, cdumez, commit-queue, ddkilzer, mitz, sam, thorton
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch.
darin: review+
Patch v2.
mitz: review+
Patch for landing. none

Description Yongjun Zhang 2015-06-29 16:26:37 PDT
When a web page starts a XHR request after the page is finished loading, we should take a background activity token and hold it until the request is cancelled or finished. Otherwise the WebContent process could be suspended since its activity token could drop to 0 after this point.
Comment 1 Yongjun Zhang 2015-06-29 16:45:09 PDT
<rdar://problem/18846532>
Comment 2 Yongjun Zhang 2015-06-29 16:52:58 PDT
Created attachment 255795 [details]
Patch.
Comment 3 Yongjun Zhang 2015-06-29 18:07:08 PDT
Hmm, this fix could have the side effect of holding the web process too long if there is a long lasting XHR request. I need to work on a better fix.
Comment 4 Yongjun Zhang 2015-06-29 18:50:16 PDT
Update the title to reflect the real problem.
Comment 5 Yongjun Zhang 2015-06-29 20:34:37 PDT
Created attachment 255810 [details]
Patch v2.
Comment 6 Simon Fraser (smfr) 2015-06-30 15:37:30 PDT
Comment on attachment 255810 [details]
Patch v2.

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

> Source/WebKit2/UIProcess/WebPageProxy.h:821
> +    void processWillBecomeForground();

"processWillBecomeForground" -> processWillBecomeForeground
Comment 7 mitz 2015-07-01 11:17:25 PDT
Comment on attachment 255810 [details]
Patch v2.

I don’t know which state transitions are possible. Is it a concern that the outcomes of the following transitions are different?
Foreground -> Suspended -> Background
Foreground -> Background

Is it a concern that this transition will fail to restore the state?
Foreground -> Suspended -> Background -> Suspended -> Foreground
(because on the second entry into Suspended, m_hasNetworkRequestsOnSuspended will change to false)
Comment 8 Yongjun Zhang 2015-07-01 11:52:20 PDT
(In reply to comment #7)
> Comment on attachment 255810 [details]
> Patch v2.
> 
> I don’t know which state transitions are possible. Is it a concern that the
> outcomes of the following transitions are different?
> Foreground -> Suspended -> Background
> Foreground -> Background

I could be wrong, but based on code inspection and my own test, I didn't find a scenario where the web process could go from Suspended to Background.

> 
> Is it a concern that this transition will fail to restore the state?
> Foreground -> Suspended -> Background -> Suspended -> Foreground
> (because on the second entry into Suspended, m_hasNetworkRequestsOnSuspended
> will change to false)

Again, this ties to whether Suspended -> Background transition is possible.

Thanks for the review! I will fix the typo on commit.
Comment 9 Yongjun Zhang 2015-07-01 15:22:25 PDT
Created attachment 255956 [details]
Patch for landing.
Comment 10 WebKit Commit Bot 2015-07-01 16:18:56 PDT
Comment on attachment 255956 [details]
Patch for landing.

Clearing flags on attachment: 255956

Committed r186200: <http://trac.webkit.org/changeset/186200>
Comment 11 WebKit Commit Bot 2015-07-01 16:19:03 PDT
All reviewed patches have been landed.  Closing bug.