RESOLVED FIXED 165917
Fix memory leak in malformed test 
https://bugs.webkit.org/show_bug.cgi?id=165917
Summary Fix memory leak in malformed test
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
DetailsFormatted DiffDiff
Patch (2.56 KB, patch)
2016-12-15 17:02 PST, Megan Gardner 		no flags
DetailsFormatted DiffDiff
Patch (2.50 KB, patch)
2016-12-15 17:09 PST, Megan Gardner 		no flags
DetailsFormatted DiffDiff
Patch (2.49 KB, patch)
2016-12-15 19:25 PST, Megan Gardner 		no flags
DetailsFormatted DiffDiff
Patch (1.81 KB, patch)
2016-12-16 15:16 PST, Megan Gardner 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
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.