Bug 165917

Summary:

Fix memory leak in malformed test

Product:

WebKit

Reporter:

Megan Gardner <megan_gardner>

Component:

New Bugs

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, bdakin, buildbot, commit-queue, megan_gardner, rniwa, simon.fraser, thorton

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews103 for mac-yosemite	none
Archive of layout-test-results from ews116 for mac-yosemite	none
Archive of layout-test-results from ews106 for mac-yosemite-wk2	none
Patch	none

Megan Gardner

Reported 2016-12-15 13:52:53 PST

Fix memory leak in malformed test

Attachments
Patch (2.60 KB, patch) 2016-12-15 16:54 PST, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (2.56 KB, patch) 2016-12-15 17:02 PST, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (2.50 KB, patch) 2016-12-15 17:09 PST, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (2.49 KB, patch) 2016-12-15 19:25 PST, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (1.81 KB, patch) 2016-12-16 15:16 PST, Megan Gardner	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.16 MB, application/zip) 2016-12-16 16:07 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews116 for mac-yosemite (2.01 MB, application/zip) 2016-12-16 16:11 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.55 MB, application/zip) 2016-12-16 16:18 PST, Build Bot	no flags	Details
Patch (2.04 KB, patch) 2016-12-19 10:36 PST, Megan Gardner	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Megan Gardner

Comment 1 2016-12-15 16:54:28 PST

Created attachment 297256 [details] Patch

Tim Horton

Comment 2 2016-12-15 16:58:52 PST

Comment on attachment 297256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297256&action=review > LayoutTests/ChangeLog:10 > + but we need to look into fixing this code-side. You should file a bug about this. Also, maybe "in WebKitTestRunner" or something instead of "code-side"; JS counts as code no matter how you feel about the language :) > LayoutTests/http/tests/quicklook/resources/tap-at-point-and-notify-done.js:6 > +function tapAtPointAndNotifyDoneGuarded(x, y) Because it calls notifyDone(), it's *never* OK to do this twice, so I think you should just guard the other one instead. (I know you're trying to limit impact, but I almost think we would probably rather find out about other tests that would be broken by this change now rather than leave them silently doing stupid things). > LayoutTests/http/tests/quicklook/resources/tap-at-point-and-notify-done.js:8 > + if ( typeof tapAtPointAndNotifyDoneGuarded.guard == 'undefined' ) { no spaces inside the parens

Megan Gardner

Comment 3 2016-12-15 17:02:28 PST

Created attachment 297259 [details] Patch

Tim Horton

Comment 4 2016-12-15 17:03:07 PST

Comment on attachment 297259 [details] Patch Most of my comments remain.

Megan Gardner

Comment 5 2016-12-15 17:09:31 PST

Created attachment 297261 [details] Patch

Tim Horton

Comment 6 2016-12-15 17:15:48 PST

Comment on attachment 297261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297261&action=review > LayoutTests/ChangeLog:8 > + Navigation caused 'onload' to be called twice, causing test harness to have extranious extraneous

Alexey Proskuryakov

Comment 7 2016-12-15 18:08:56 PST

Comment on attachment 297261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297261&action=review > LayoutTests/ChangeLog:9 > + HID callback around after script controller was dismantled. Guard should fix test, I think that a cleaner way to fix this would be to move the onload attribute to main frame body. We normally don't want tests to run before the main frame is done loading.

Megan Gardner

Comment 8 2016-12-15 19:25:05 PST

Created attachment 297288 [details] Patch

Alexey Proskuryakov

Comment 9 2016-12-16 09:21:22 PST

Comment on attachment 297288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297288&action=review > LayoutTests/ChangeLog:8 > + Navigation caused 'onload' to be called twice, causing test harness to have extraneous Would you be willing to look into the other approach that I proposed above?

Megan Gardner

Comment 10 2016-12-16 12:03:17 PST

Oh, sorry Alexey, I didn't see your suggestion amid the churn.

Megan Gardner

Comment 11 2016-12-16 15:16:44 PST

Created attachment 297363 [details] Patch

Simon Fraser (smfr)

Comment 12 2016-12-16 15:52:57 PST

Comment on attachment 297363 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297363&action=review > LayoutTests/http/tests/quicklook/hide-referer-on-navigation.html:14 > +<body onload="tapAtPointAndNotifyDone(document.getElementsByTagName('iframe')[0].offsetLeft > + + 5, document.getElementsByTagName('iframe')[0].offsetTop + 5)"> Would be nicer to say onload="doTest()" and have function doTest()... in the JS above.

Build Bot

Comment 13 2016-12-16 16:07:28 PST

Comment on attachment 297363 [details] Patch Attachment 297363 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2739798 New failing tests: http/tests/navigation/keyboard-events-during-provisional-navigation.html

Build Bot

Comment 14 2016-12-16 16:07:31 PST

Created attachment 297369 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5

Build Bot

Comment 15 2016-12-16 16:11:40 PST

Comment on attachment 297363 [details] Patch Attachment 297363 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2739788 New failing tests: http/tests/navigation/keyboard-events-during-provisional-navigation.html

Build Bot

Comment 16 2016-12-16 16:11:44 PST

Created attachment 297370 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5

Build Bot

Comment 17 2016-12-16 16:18:35 PST

Comment on attachment 297363 [details] Patch Attachment 297363 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2739838 New failing tests: http/tests/navigation/keyboard-events-during-provisional-navigation.html

Build Bot

Comment 18 2016-12-16 16:18:38 PST

Created attachment 297371 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Alexey Proskuryakov

Comment 19 2016-12-16 16:38:32 PST

The http/tests/navigation/keyboard-events-during-provisional-navigation.html failures are unrelated.

Megan Gardner

Comment 20 2016-12-16 17:22:25 PST

so... can this get pushed anyways then? this is an iOS test, it can't be causing Mac failures...

Tim Horton

Comment 21 2016-12-16 17:23:20 PST

(In reply to comment #20) > so... can this get pushed anyways then? this is an iOS test, it can't be > causing Mac failures... We will try to outsmart the bots.

Megan Gardner

Comment 22 2016-12-19 10:36:12 PST

Created attachment 297466 [details] Patch

Alexey Proskuryakov

Comment 23 2016-12-20 09:01:27 PST

Comment on attachment 297466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297466&action=review > LayoutTests/http/tests/quicklook/hide-referer-on-navigation.html:7 > + tapAtPointAndNotifyDone(document.getElementsByTagName('iframe')[0].offsetLeft + 5, document.getElementsByTagName('iframe')[0].offsetTop + 5) It would be slightly nicer to store the result of document.getElementsByTagName('iframe')[0] in a local variable.

WebKit Commit Bot

Comment 24 2016-12-20 09:26:07 PST

Comment on attachment 297466 [details] Patch Clearing flags on attachment: 297466 Committed r210022: <http://trac.webkit.org/changeset/210022>

WebKit Commit Bot

Comment 25 2016-12-20 09:26:13 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.