Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
516843004:
Fix crash when accessing Event::path(). (Closed)
DescriptionFix crash when accessing Event::path().
(A more elaborate account of the details is found in the bug report.)
BUG=400476
R=haraken@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181200
Patch Set 1 #Patch Set 2 : Added regression test. #
Total comments: 22
Patch Set 3 : More work on the test case. #
Total comments: 4
Patch Set 4 : Review feedback. #
Messages
Total messages: 19 (1 generated)
LGTM, thanks for the fix!
oh, can you add the test case in the bug?
Added test. PTAL.
Please refer to other layout tests that use js-test.js and follow the coding style. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... File LayoutTests/fast/dom/event-400476.html (right): https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:1: <html> Make the test name more descriptive. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:3: <script> You can include resources/js-test.js, which has a bunch of utility functions for testing. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:5: testRunner.dumpAsText(); Once you include js-test.js, testRunner.dumpAsText() won't be needed. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:10: root=document.documentElement; Use 4-indent space. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:10: root=document.documentElement; Add 'var' to local variables. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:11: iframe=root.ownerDocument.createElementNS('http://www.w3.org/1999/xhtml','iframe'); Add one space around '='. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:12: iframe.onload=iframe_onload_fn; iframe_onload_fn => iframeLoaded Use camelCase names. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:15: function iframe_onload_fn() { Add one empty line between functions. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:19: window.setTimeout(triggerFn, 100); triggerFn => nextIframeLoaded Use a more descriptive name. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:25: document.write('<p>' + problem_ref.path + '</p>'); Don't use document.write unless it's really needed. Here you can use shouldBe('event.path', ...) https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:29: </script> Put this <script> tag in <body>. Then you won't need the test() function.
PTAL. I did quite a bit of fiddling with the test case and never quite managed to stabilize it. I'm not that sure it will be useful... In more detail: The regression test now works, but is rather flaky. In the sense that it may pass if the bug is present. If it does fail, then the bug is real. So, no false negatives; but the false positives will make it useless for e.g. bisect. There are several features that seem to increase the frequency of the bug occurring, but I never found a version that reliably exposes the bug. - ASAN: Definitely helps exposing the bug. But it may also crash without ASAN, it's just less likely. - gc(): Crashes with zero, one, or many gc() calls. With more gc cycles the likelihood of a crash seems to increase. I suspect that these effectively act as a delay loop. - Increasing the timeout: Seems to have an effect, but not big. So, honestly, I'm a bit unsure whether its worth the effort. Any feedback would be welcome. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... File LayoutTests/fast/dom/event-400476.html (right): https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:1: <html> On 2014/08/28 17:32:39, haraken wrote: > > Make the test name more descriptive. Done. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:3: <script> On 2014/08/28 17:32:39, haraken wrote: > > You can include resources/js-test.js, which has a bunch of utility functions for > testing. > Done. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:5: testRunner.dumpAsText(); On 2014/08/28 17:32:39, haraken wrote: > > Once you include js-test.js, testRunner.dumpAsText() won't be needed. Done. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:10: root=document.documentElement; On 2014/08/28 17:32:38, haraken wrote: > > Use 4-indent space. Done. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:10: root=document.documentElement; On 2014/08/28 17:32:39, haraken wrote: > > Add 'var' to local variables. Done. (Here & elsewhere.) https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:11: iframe=root.ownerDocument.createElementNS('http://www.w3.org/1999/xhtml','iframe'); On 2014/08/28 17:32:38, haraken wrote: > > Add one space around '='. Done. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:12: iframe.onload=iframe_onload_fn; On 2014/08/28 17:32:39, haraken wrote: > > iframe_onload_fn => iframeLoaded > > Use camelCase names. Done. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:15: function iframe_onload_fn() { On 2014/08/28 17:32:39, haraken wrote: > > Add one empty line between functions. Done. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:19: window.setTimeout(triggerFn, 100); On 2014/08/28 17:32:38, haraken wrote: > > triggerFn => nextIframeLoaded > > Use a more descriptive name. Done. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:25: document.write('<p>' + problem_ref.path + '</p>'); On 2014/08/28 17:32:39, haraken wrote: > > Don't use document.write unless it's really needed. Here you can use > shouldBe('event.path', ...) Done. https://codereview.chromium.org/516843004/diff/20001/LayoutTests/fast/dom/eve... LayoutTests/fast/dom/event-400476.html:29: </script> On 2014/08/28 17:32:39, haraken wrote: > > Put this <script> tag in <body>. Then you won't need the test() function. Done. (Well, it's still in the header, but the onload=... is gone.)
LGTM. As long as there are no false negatives, it's worth having the test. Does this test need to be added to http/tests/ ? If no, let's move it to fast/dom/. https://codereview.chromium.org/516843004/diff/40001/LayoutTests/http/tests/d... File LayoutTests/http/tests/dom/crash-on-querying-event-path.html (right): https://codereview.chromium.org/516843004/diff/40001/LayoutTests/http/tests/d... LayoutTests/http/tests/dom/crash-on-querying-event-path.html:5: var jsTestIsAsync = true; Nit: We normally don't indent the top-level block of JavaScript. https://codereview.chromium.org/516843004/diff/40001/LayoutTests/http/tests/d... LayoutTests/http/tests/dom/crash-on-querying-event-path.html:41: </script> Nit: You can put the script into the <body>.
On Sun, Aug 31, 2014 at 8:54 AM, <haraken@chromium.org> wrote: > Does this test need to be added to http/tests/ ? If no, let's move it to > fast/dom/. > I think it needs to be in http/tests/. Not quite sure why, but there's an issue loading iframes from file:-urls. I suspect the browser blocks the load and then the test case no longer triggers the bug. I think there's a command-line switch to to allow this, but I don't think the fast tests use this. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
> > Does this test need to be added to http/tests/ ? If no, let's move it to > > fast/dom/. > > > > I think it needs to be in http/tests/. Not quite sure why, but there's an > issue loading iframes from file:-urls. I suspect the browser blocks the > load and then the test case no longer triggers the bug. I think there's a > command-line switch to to allow this, but I don't think the fast tests use > this. I guess there are a lot of iframe related tests in fast/dom/. What happens if you put the test in fast/dom/?
https://codereview.chromium.org/516843004/diff/40001/LayoutTests/http/tests/d... File LayoutTests/http/tests/dom/crash-on-querying-event-path.html (right): https://codereview.chromium.org/516843004/diff/40001/LayoutTests/http/tests/d... LayoutTests/http/tests/dom/crash-on-querying-event-path.html:5: var jsTestIsAsync = true; On 2014/08/31 06:54:15, haraken wrote: > > Nit: We normally don't indent the top-level block of JavaScript. Done. https://codereview.chromium.org/516843004/diff/40001/LayoutTests/http/tests/d... LayoutTests/http/tests/dom/crash-on-querying-event-path.html:41: </script> On 2014/08/31 06:54:15, haraken wrote: > > Nit: You can put the script into the <body>. Done.
On Mon, Sep 1, 2014 at 4:28 PM, <haraken@chromium.org> wrote: > > Does this test need to be added to http/tests/ ? If no, let's move it to >> > fast/dom/. >> > >> > > I think it needs to be in http/tests/. Not quite sure why, but there's an >> issue loading iframes from file:-urls. I suspect the browser blocks the >> load and then the test case no longer triggers the bug. I think there's a >> command-line switch to to allow this, but I don't think the fast tests use >> this. >> > > I guess there are a lot of iframe related tests in fast/dom/. What happens > if > you put the test in fast/dom/? > content_shell binaries were identical. Test files were identical, except the include path for js-test.js was adapted. http/tests/dom/..: 2 crashes in 6 runs. fast/dom/...: 0 crashes in 6 runs. I don't /really/ have an explanation for this behaviour, and since N-out-of-6 runs is statistics with low numbers I also don't /really/ have evidence that http is necessary, but it certainly does look that way. Please do advise what you want me to look for. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2014/09/01 14:53:45, vogelheim wrote: > On Mon, Sep 1, 2014 at 4:28 PM, <mailto:haraken@chromium.org> wrote: > > > > Does this test need to be added to http/tests/ ? If no, let's move it to > >> > fast/dom/. > >> > > >> > > > > I think it needs to be in http/tests/. Not quite sure why, but there's an > >> issue loading iframes from file:-urls. I suspect the browser blocks the > >> load and then the test case no longer triggers the bug. I think there's a > >> command-line switch to to allow this, but I don't think the fast tests use > >> this. > >> > > > > I guess there are a lot of iframe related tests in fast/dom/. What happens > > if > > you put the test in fast/dom/? > > > > content_shell binaries were identical. Test files were identical, except > the include path for js-test.js was adapted. > > http/tests/dom/..: 2 crashes in 6 runs. > fast/dom/...: 0 crashes in 6 runs. > > I don't /really/ have an explanation for this behaviour, and since > N-out-of-6 runs is statistics with low numbers I also don't /really/ have > evidence that http is necessary, but it certainly does look that way. > Please do advise what you want me to look for. I guess it will be related to the delay of loading the resource (It takes time to load the resource from an http server). Can you try to insert setTimeout somewhere to delay loading the resource and make the test more reliable?
On Mon, Sep 1, 2014 at 4:55 PM, <haraken@chromium.org> wrote: > On 2014/09/01 14:53:45, vogelheim wrote: > > On Mon, Sep 1, 2014 at 4:28 PM, <mailto:haraken@chromium.org> wrote: >> > > > > Does this test need to be added to http/tests/ ? If no, let's move it >> to >> >> > fast/dom/. >> >> > >> >> >> > >> > I think it needs to be in http/tests/. Not quite sure why, but there's >> an >> >> issue loading iframes from file:-urls. I suspect the browser blocks the >> >> load and then the test case no longer triggers the bug. I think >> there's a >> >> command-line switch to to allow this, but I don't think the fast tests >> use >> >> this. >> >> >> > >> > I guess there are a lot of iframe related tests in fast/dom/. What >> happens >> > if >> > you put the test in fast/dom/? >> > >> > > content_shell binaries were identical. Test files were identical, except >> the include path for js-test.js was adapted. >> > > http/tests/dom/..: 2 crashes in 6 runs. >> fast/dom/...: 0 crashes in 6 runs. >> > > I don't /really/ have an explanation for this behaviour, and since >> N-out-of-6 runs is statistics with low numbers I also don't /really/ have >> evidence that http is necessary, but it certainly does look that way. >> Please do advise what you want me to look for. >> > > I guess it will be related to the delay of loading the resource (It takes > time > to load the resource from an http server). > > Can you try to insert setTimeout somewhere to delay loading the resource > and > make the test more reliable? > Well, I had briefly discussed that earlier in this thread, and the timeout didn't seem to make much of a difference. I now set the timeout to 5s, which is substantially slower than loading a ~1kB document from a local http server should be and I still get 0 crashes (in 7 runs, this time). I don't think the difference in reproducing the test case can be explained solely by timing. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
OK, then let's put the test in http/tests/ and land. LGTM. It's an unfortunate that we cannot have a reliable test, but it would be more important to land the fix asap. Thanks for the fix!
The CQ bit was checked by vogelheim@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vogelheim@chromium.org/516843004/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/25130)
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 181200
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/532593002/ by tkent@chromium.org. The reason for reverting is: The test crash-on-querying-event-path.html is unacceptably flaky. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=@To... . |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
