Bug 135634 – URLs sometimes remain in IconDatabase files after history is cleared.

• Component: History
• Status: RESOLVED FIXED

[Gordon Sheridan]

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.

[Gordon Sheridan]

Created attachment 236076 [details]
Patch

[WebKit Commit Bot]

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.

[Gordon Sheridan]

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.

[Gordon Sheridan]

I will also be working on API tests for these methods.

[Brady Eidson]

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) { }

[Alexey Proskuryakov]

> 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.

[Gordon Sheridan]

Thanks for the great feedback. I'll update the patch and resubmit.

[Jessie Berlin]

(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.

[Alexey Proskuryakov]

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?

[Brady Eidson]

(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.

[Gordon Sheridan]

Here's the radar bug: <rdar://problem/17388461>

[Gordon Sheridan]

Created attachment 236162 [details]
Patch

[Gordon Sheridan]

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.

[Brady Eidson]

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.

[Gordon Sheridan]

Created attachment 236205 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 236205 [details]
Patch

Clearing flags on attachment: 236205

Committed r172235: <http://trac.webkit.org/changeset/172235>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.