Bug 117106 - [wk2] WebProcess' page-in-window count can be wrong
Summary: [wk2] WebProcess' page-in-window count can be wrong
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-06-01 01:05 PDT by Tim Horton
Modified: 2013-06-05 12:58 PDT (History)
3 users (show)

See Also:


Attachments
patch (5.82 KB, patch)
2013-06-01 01:17 PDT, Tim Horton
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2013-06-01 01:05:29 PDT
Interestingly:

Close-tab hits both WebPage::setIsInWindow(false) and WebPage::close().

Close-window only hits WebPage::close().

I didn't add decrement-page-in-window count to WebPage::close() :(

So, opening and closing a window is an easy way to make sure we never do cleanup on non-PPT.

I have a patch that turns the page-in-window thing into a hashset of pageids instead of a count so we can decrement from both setIsInWindow(false) and WebPage::close.
Comment 1 Radar WebKit Bug Importer 2013-06-01 01:05:46 PDT
<rdar://problem/14040991>
Comment 2 Tim Horton 2013-06-01 01:17:59 PDT
Created attachment 203483 [details]
patch
Comment 3 Tim Horton 2013-06-01 01:18:55 PDT
Comment on attachment 203483 [details]
patch

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

> Source/WebKit2/ChangeLog:9
> +        If we're using a single WebProcess and a window is closed, we were

Maybe I should remove the bit about single WebProcess since there are cases where this can affect multiprocess too (if we're over the process cap or are using window.open or something).
Comment 4 Simon Fraser (smfr) 2013-06-01 09:51:10 PDT
Comment on attachment 203483 [details]
patch

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

> Source/WebKit2/WebProcess/WebProcess.h:316
> +    HashSet<uint64_t> m_inWindowPageSet;

inWindowPageSet sounds like something boolean. I guess this is "set of pages in any window"?
Comment 5 Tim Horton 2013-06-02 02:22:00 PDT
(In reply to comment #4)
> (From update of attachment 203483 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203483&action=review
> 
> > Source/WebKit2/WebProcess/WebProcess.h:316
> > +    HashSet<uint64_t> m_inWindowPageSet;
> 
> inWindowPageSet sounds like something boolean. I guess this is "set of pages in any window"?

It is "set of pages in this process that are parented in a window", yes.
Comment 6 Anders Carlsson 2013-06-02 17:07:30 PDT
Comment on attachment 203483 [details]
patch

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

>>> Source/WebKit2/WebProcess/WebProcess.h:316
>>> +    HashSet<uint64_t> m_inWindowPageSet;
>> 
>> inWindowPageSet sounds like something boolean. I guess this is "set of pages in any window"?
> 
> It is "set of pages in this process that are parented in a window", yes.

How about m_pagesInWindows or something similar. I don’t think you need to put the type in the variable name.
Comment 7 Tim Horton 2013-06-05 12:58:03 PDT
http://trac.webkit.org/changeset/151235