Bug 135634 - URLs sometimes remain in IconDatabase files after history is cleared.
Summary: URLs sometimes remain in IconDatabase files after history is cleared.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Unspecified
: P1 Normal
Assignee: Gordon Sheridan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-08-05 19:24 PDT by Gordon Sheridan
Modified: 2014-08-08 08:38 PDT (History)
7 users (show)

See Also:


Attachments
Patch (7.41 KB, patch)
2014-08-05 20:05 PDT, Gordon Sheridan
no flags Details | Formatted Diff | Diff
Patch (7.61 KB, patch)
2014-08-06 18:53 PDT, Gordon Sheridan
no flags Details | Formatted Diff | Diff
Patch (7.00 KB, patch)
2014-08-07 12:13 PDT, Gordon Sheridan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gordon Sheridan 2014-08-05 19:24:24 PDT
When HistoryItem objects, which retain and release the page URLs for favicons in their constructors/destructors, are removed from the WebBackForwardList, it is possible for one or more HistoryControllers to retain a reference to the HistoryItem in its m_previousItem member. This prevents the destruction of the HistoryItem, so the URL is not released from the IconDatabase.
Comment 1 Gordon Sheridan 2014-08-05 20:05:00 PDT
Created attachment 236076 [details]
Patch
Comment 2 WebKit Commit Bot 2014-08-05 20:05:56 PDT
Attachment 236076 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Gordon Sheridan 2014-08-05 20:10:22 PDT
I chose to add a general purpose call to WebCore::Page, to execute a function or closure for each Page. If it is preferable to create a more narrow function with the sole purpose of clearing the m_previousItem member of the HistoryControllers associated with each Page, I'd be happy to provide a new patch.

I also welcome naming suggestions for the methods.
Comment 4 Gordon Sheridan 2014-08-05 20:11:01 PDT
I will also be working on API tests for these methods.
Comment 5 Brady Eidson 2014-08-05 20:51:08 PDT
Comment on attachment 236076 [details]
Patch

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

In general, looks good.

> I chose to add a general purpose call to WebCore::Page, to execute a function or closure for each Page.

I think we should be more narrow and add a function specific to this.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

I think we need an API test for this.

> Source/WebCore/page/Page.cpp:278
> +    for (auto it = allPages->begin(), end = allPages->end(); it != end; ++it) {
> +        if (callBack(*it))
> +            return;
> +    }

This should be a C++11 for loop.

for (auto& page : *allPages) { }
Comment 6 Alexey Proskuryakov 2014-08-05 22:55:32 PDT
> I think we should be more narrow and add a function specific to this.

+many

This gives callers a huge huge gun to shoot themselves in the foot. Calling arbitrary functions during iteration could easily result in the container being modified, and iterators becoming invalid. The allPages container is private to Page, so Page should know about everything that's being done with it.
Comment 7 Gordon Sheridan 2014-08-06 11:24:43 PDT
Thanks for the great feedback. I'll update the patch and resubmit.
Comment 8 Jessie Berlin 2014-08-06 12:46:52 PDT
(In reply to comment #5)
> (From update of attachment 236076 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236076&action=review
> 
> In general, looks good.
> 
> > I chose to add a general purpose call to WebCore::Page, to execute a function or closure for each Page.
> 
> I think we should be more narrow and add a function specific to this.
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests (OOPS!).
> 
> I think we need an API test for this.

I talked with Brady, and he confirmed this would need a way to run a http server during the test (visit multiple pages with multiple favicons).

It is unclear to me whether we have that ability today with our API tests. If we don't, we shouldn't make adding it be part of this patch.
Comment 9 Alexey Proskuryakov 2014-08-06 12:55:37 PDT
Comment on attachment 236076 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=135634

Is there a Radar bug for this?
Comment 10 Brady Eidson 2014-08-06 13:07:20 PDT
(In reply to comment #9)
> (From update of attachment 236076 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236076&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=135634
> 
> Is there a Radar bug for this?

Good call - Yes, there is.  Gordon should link to the rdar: here and add the InRadar keyword.
Comment 11 Gordon Sheridan 2014-08-06 13:29:49 PDT
Here's the radar bug: <rdar://problem/17388461>
Comment 12 Gordon Sheridan 2014-08-06 18:53:18 PDT
Created attachment 236162 [details]
Patch
Comment 13 Gordon Sheridan 2014-08-06 19:00:48 PDT
Comment on attachment 236162 [details]
Patch

Updated patch to rename and restrict purpose of method added to WebCore::Page to clear the m_previousItem of HistoryController associated with the main frame when it matches the HistoryItem being removed from the back/forward list. HistoryController::clearPreviousItem() also clears the m_previousItem in the HistoryControllers associated with any descendants in the FrameTree.
Comment 14 Brady Eidson 2014-08-07 11:15:48 PDT
Comment on attachment 236162 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        No API tests were added because they would require running an http server during the test
> +        to visit multiple sites to download favicons and generate particular history sequences.

This is a little wordy and not quite right.

I would just say "No new tests.  Would require an API test that also needs an httpd, which we don't currently support."

> Source/WebCore/ChangeLog:13
> +        Added exports for HistoryController::clearPreviousItem and Page::clearPreviousItemFromAllPages.

I don't think you need the HistoryController::clearPreviousItem export.

> Source/WebCore/WebCore.exp.in:757
> +__ZN7WebCore17HistoryController17clearPreviousItemEv

Don't think you need this export - WK2 doesn't call it, it's only called within WebCore.
Comment 15 Gordon Sheridan 2014-08-07 12:13:27 PDT
Created attachment 236205 [details]
Patch
Comment 16 WebKit Commit Bot 2014-08-07 13:21:02 PDT
Comment on attachment 236205 [details]
Patch

Clearing flags on attachment: 236205

Committed r172235: <http://trac.webkit.org/changeset/172235>
Comment 17 WebKit Commit Bot 2014-08-07 13:21:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Darin Adler 2014-08-08 08:38:57 PDT
Comment on attachment 236076 [details]
Patch

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

> Source/WebCore/loader/HistoryController.cpp:604
> +    m_previousItem = nullptr;
> +    for (Frame* child = m_frame.tree().firstChild(); child; child = child->tree().nextSibling())
> +        child->loader().history().clearPreviousItem();

It’s nicer to do this in an iterative way. Something like this:

    for (Frame* frame = &m_frame.get(); frame; frame = frame->tree().traverseNext(&m_frame.get()))
        frame->loader().history().m_previousItem = nullptr;

It’ll be even more elegant later when we have iterators that work on the frame tree. Someone was working on a patch for that.

It’s also a little ugly that this function exists for all history controllers but it’s really only appropriate for top level ones. It would not be right to call this on a subframe. If we knew this was a main frame then we would not need to pass m_frame to traverseNext.

> Source/WebCore/page/Page.cpp:270
> +void Page::forEach(const std::function<bool(Page*)>& callBack)

This is a new coding style for WebKit. We don’t have any other forEach functions, and the style where we return a bool to indicate we should stop iterating is also new.

I think this should just be a std::function, not a const std::function&.

The function should take a Page& argument, not a Page*.

The name callBack is not a good argument name because it is a verb phrase, not a noun phrase. Even just naming it “function” would be better.

> Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:142
> +        if (item == controller.previousItem()) {
> +            controller.clearPreviousItem();
> +            return true;
> +        }
> +        return false;

Would read slightly better in early-return style, meaning we’d return in the != case before doing the additional work on the normal case. We want to minimize the nesting of the real work.