Bug 185322 - ASSERT(!childItemWithTarget(child->target())) is hit in HistoryItem::addChildItem()
Summary: ASSERT(!childItemWithTarget(child->target())) is hit in HistoryItem::addChild...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://build.webkit.org/results/Appl...
Keywords: InRadar
Depends on:
Blocks: 11388
  Show dependency treegraph
 
Reported: 2018-05-04 13:22 PDT by Chris Dumez
Modified: 2018-05-07 18:54 PDT (History)
7 users (show)

See Also:


Attachments
Patch (724.60 KB, patch)
2018-05-04 18:51 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.22 MB, application/zip)
2018-05-04 20:03 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (8.57 MB, application/zip)
2018-05-04 20:42 PDT, EWS Watchlist
no flags Details
Patch (727.42 KB, patch)
2018-05-04 20:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (727.50 KB, patch)
2018-05-07 11:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-05-04 13:22:59 PDT
ASSERT(!childItemWithTarget(child->target())) is hit in HistoryItem::addChildItem():
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000014faad070 WTFCrash + 16 (Assertions.cpp:261)
1   com.apple.WebCore             	0x000000014213064d WebCore::HistoryItem::addChildItem(WTF::Ref<WebCore::HistoryItem, WTF::DumbPtrTraits<WebCore::HistoryItem> >&&) + 253 (HistoryItem.cpp:316)
2   com.apple.WebCore             	0x000000014264c7c0 WebCore::HistoryController::createItemTree(WebCore::Frame&, bool) + 416 (HistoryController.cpp:718)
3   com.apple.WebCore             	0x0000000142649990 WebCore::HistoryController::updateBackForwardListClippedAtTarget(bool) + 208 (HistoryController.cpp:821)
4   com.apple.WebCore             	0x000000014264ae7f WebCore::HistoryController::updateForStandardLoad(WebCore::HistoryController::HistoryUpdateType) + 335 (HistoryController.cpp:392)
5   com.apple.WebCore             	0x000000014262879a WebCore::FrameLoader::transitionToCommitted(WebCore::CachedPage*) + 1082 (FrameLoader.cpp:2069)
6   com.apple.WebCore             	0x000000014262787e WebCore::FrameLoader::commitProvisionalLoad() + 2142 (FrameLoader.cpp:1892)
7   com.apple.WebCore             	0x00000001425d095c WebCore::DocumentLoader::commitIfReady() + 60 (DocumentLoader.cpp:356)
8   com.apple.WebCore             	0x00000001425d6aac WebCore::DocumentLoader::commitLoad(char const*, int) + 76 (DocumentLoader.cpp:958)
9   com.apple.WebCore             	0x00000001425d6a4f WebCore::DocumentLoader::dataReceived(char const*, int) + 511 (DocumentLoader.cpp:1107)
10  com.apple.WebCore             	0x00000001425d7204 WebCore::DocumentLoader::dataReceived(WebCore::CachedResource&, char const*, int) + 116 (DocumentLoader.cpp:1080)

The unique frame names we generate are based on their position inside their parent and is not properly updated when the tree structure changes. As a result, we can end up with duplicate uniqueFrameNames and hit this assertion.
Comment 1 Chris Dumez 2018-05-04 14:53:22 PDT
The issue seems to be:
1. We have a top frame with 8 subframes
-> Frame names are frame0 to frame7 based on their position in their parent
2. Then a new frame is inserted *before* the last one
-> it gets frame7 name and existing frame7 is NOT renamed to frame8. Therfore, we end up with a conflict.
Comment 2 Chris Dumez 2018-05-04 18:51:22 PDT
Created attachment 339621 [details]
Patch
Comment 3 EWS Watchlist 2018-05-04 20:03:39 PDT
Comment on attachment 339621 [details]
Patch

Attachment 339621 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7571797

New failing tests:
webarchive/loading/object.html
Comment 4 EWS Watchlist 2018-05-04 20:03:41 PDT
Created attachment 339624 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 5 EWS Watchlist 2018-05-04 20:42:53 PDT
Comment on attachment 339621 [details]
Patch

Attachment 339621 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7571830

New failing tests:
webarchive/test-duplicate-resources.html
webarchive/adopt-attribute-styled-body-webarchive.html
Comment 6 EWS Watchlist 2018-05-04 20:42:54 PDT
Created attachment 339628 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 7 Chris Dumez 2018-05-04 20:55:58 PDT
Created attachment 339630 [details]
Patch
Comment 8 Geoffrey Garen 2018-05-07 10:12:37 PDT
Comment on attachment 339630 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:10
> +        and for things like restoring form state from an HistoryItem.

a HistoryItem

> Source/WebCore/ChangeLog:15
> +        would not take care of updating existing Frames's unique name on frame tree mutation.

Frames'
Comment 9 WebKit Commit Bot 2018-05-07 10:42:32 PDT
Comment on attachment 339630 [details]
Patch

Rejecting attachment 339630 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 339630, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
fs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 231439 = ee4ed66a0af3fd7fb8e7ee50defa9539e558fb05
r231440 = 9d20703ea1ea299423fc94fa79a632ba25479acd
r231441 = 9ab072da44e00798d9743d500585940a3f4332f9
r231442 = 79933c0d14e2a3dbc9f8abe637575ac1ef9362bc
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/7596117
Comment 10 Chris Dumez 2018-05-07 11:52:10 PDT
Created attachment 339736 [details]
Patch
Comment 11 Chris Dumez 2018-05-07 11:55:31 PDT
Comment on attachment 339736 [details]
Patch

Clearing flags on attachment: 339736

Committed r231450: <https://trac.webkit.org/changeset/231450>
Comment 12 Chris Dumez 2018-05-07 11:55:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-05-07 11:56:44 PDT
<rdar://problem/40031223>
Comment 14 Ryan Haddad 2018-05-07 17:52:40 PDT
This change appears to have caused performance test failures. Dromaeo and Speedometer fail with ERROR: frame "<!--frame7-->" - has 1 onunload handler(s)
https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Perf%29/builds/772
Comment 15 Chris Dumez 2018-05-07 18:32:12 PDT
(In reply to Ryan Haddad from comment #14)
> This change appears to have caused performance test failures. Dromaeo and
> Speedometer fail with ERROR: frame "<!--frame7-->" - has 1 onunload
> handler(s)
> https://build.webkit.org/builders/
> Apple%20Sierra%20Release%20WK2%20%28Perf%29/builds/772

Looking.
Comment 16 Chris Dumez 2018-05-07 18:35:55 PDT
(In reply to Chris Dumez from comment #15)
> (In reply to Ryan Haddad from comment #14)
> > This change appears to have caused performance test failures. Dromaeo and
> > Speedometer fail with ERROR: frame "<!--frame7-->" - has 1 onunload
> > handler(s)
> > https://build.webkit.org/builders/
> > Apple%20Sierra%20Release%20WK2%20%28Perf%29/builds/772
> 
> Looking.

Oh, Tools/Scripts/webkitpy/performance_tests/perftest.py needs updating for new naming. Will land a fix shortly.
Comment 17 Chris Dumez 2018-05-07 18:54:38 PDT
(In reply to Chris Dumez from comment #16)
> (In reply to Chris Dumez from comment #15)
> > (In reply to Ryan Haddad from comment #14)
> > > This change appears to have caused performance test failures. Dromaeo and
> > > Speedometer fail with ERROR: frame "<!--frame7-->" - has 1 onunload
> > > handler(s)
> > > https://build.webkit.org/builders/
> > > Apple%20Sierra%20Release%20WK2%20%28Perf%29/builds/772
> > 
> > Looking.
> 
> Oh, Tools/Scripts/webkitpy/performance_tests/perftest.py needs updating for
> new naming. Will land a fix shortly.

<https://trac.webkit.org/changeset/231474>