Bug 45737 – [Chromium] fast/frames/frame-limit.html is slow on debug bots

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Tools / Tests

[Pavel Podivilov]

[Chromium] fast/frames/frame-limit.html is crashing on debug bots after r67179:r67353 roll

[Pavel Podivilov]

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fframes%2Fframe-limit.html

[Pavel Podivilov]

Created attachment 67789 [details]
Proposed patch.

Regressed in r67182. In HTMLFrameOwnerElement::willRemove: frame->loader()->frameDetached() causes content frame destruction, then frame->disconnectOwnerElement() crashes.

[Dimitri Glazkov (Google)]

Comment on attachment 67789 [details]
Proposed patch.

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

> WebCore/html/HTMLFrameOwnerElement.cpp:57
> +        RefPtr<Frame> protect(frame);

Aw crap, frameDetached calls FrameLoader::detachFromParent(), which in turn may destroy the frame. Good catch. 

I see that this is necessary, I just don't like that we have to double-protect it (once here and once in detachFromParent). I also don't like that we end up calling disconnectedOwnerElement twice in such cases.

[WebKit Commit Bot]

Comment on attachment 67789 [details]
Proposed patch.

Rejecting patch 67789 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--quiet']" exit_code: 1
Last 500 characters of output:
sts
Testing 21299 test cases.
websocket/tests/bad-sub-protocol-empty.html -> timed out
Sampling process 27542 for 10 seconds with 10 milliseconds of run time between samples
Sampling completed, processing symbols...
Sample analysis of process 27542 written to file /Users/eseidel/Library/Logs/DumpRenderTree/HangReport.txt

Exiting early after 1 failures. 21254 tests run.
562.20s total testing time

21253 test cases (99%) succeeded
1 test case (<1%) timed out
33 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/4042030

[Eric Seidel (no email)]

Comment on attachment 67789 [details]
Proposed patch.

I believe this test is just flaky.

[WebKit Commit Bot]

Comment on attachment 67789 [details]
Proposed patch.

Clearing flags on attachment: 67789

Committed r67659: <http://trac.webkit.org/changeset/67659>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67659 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/67659
http://trac.webkit.org/changeset/67660

[Eric Seidel (no email)]

Filed bug 45918.

[Eric Seidel (no email)]

I reordered the two calls in this function in an earlier change. I bet that caused this. I'm not sure using a protect ptr is right here.

[Eric Seidel (no email)]

*** Bug 45804 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

I dont' think it's right to add a Protect ptr w/o a comment as to what it's protecting against.  In this case, it's not clear from reading the code why it's needed.

Clearly my re-ordering of those calls in bug 45365 caused this bug.  However before I reordered them the frame counts were off during frameDetached.

            (WebCore::HTMLFrameOwnerElement::willRemove):
             - Disconnecting the owner element removes the frame from the frame tree.
               frameDetached() calls Page::frameCount which expects that the frame is
               already gone at this point and asserts when it's not.  It's unclear how
               this worked before, except that the frame removal was likely done in the
               post-attach callback, so the frameCount was wrong (too high) during
               frameDetached(), but was fixed up in the post-detach callback.

Maybe my re-ordering was wrong.

[Pavel Podivilov]

[Chromium] fast/frames/frame-limit.html is slow on debug bots

[Abhishek Arya]

Pavel, can you please take a look into this. This is a high severity security bug that we dont want to make into v7 (and this is also a regression).

[Abhishek Arya]

I had a chat with Pavel on this. He dont understand this code path and his attempt was a patch to fix the failure on the bots. It did fix things on the bots and also crash didn't reproduce with my testcase. But someone who understands this code, needs to be check if that is ok and functionally the right thing to do.

Eric, i dont know how to process with regards to this. Any advise on the owner for this. Or do you think it is ok to merge this fix for the chromium v7 release.

[Adam Klein]

Updated as always timing out on debug builds in http://trac.webkit.org/changeset/109420

[Julien Chaffraix]

*** Bug 98919 has been marked as a duplicate of this bug. ***

[Stephen Chenney]

Marking test failures as WontFix. Bug is still accessible and recording in TestExpectations.