http/tests/security/xss-eval.html started failing on all JavaScriptCore platforms after r129456: http://trac.webkit.org/changeset/129456 http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=http%2Ftests%2Fsecurity%2Fxss-eval.html Here's the diff: --- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/http/tests/security/xss-eval-expected.txt +++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/http/tests/security/xss-eval-actual.txt @@ -5,7 +5,7 @@ If the test passes, you'll see a pass message below. PASS: eval.call(frames[0], 'document') should be EvalError and is. -PASS: childEval.call(frames[0], 'document') should be EvalError and is. +FAIL: childEval.call(frames[0], 'document') should be EvalError but instead is [object HTMLDocument]. PASS: childEvalCaller('document') should be TypeError and is. -PASS: childLocalEvalCaller('document') should be EvalError and is. +FAIL: childLocalEvalCaller('document') should be EvalError but instead is [object HTMLDocument].
Ooops, I'll look into this! - we may be changing this behaviour anyway, I don't think these EvalErrors match the spec, going to test against other browsers.
Skipped on Qt by https://trac.webkit.org/changeset/129481, on GTK by https://trac.webkit.org/changeset/129460
And on EFL in http://trac.webkit.org/changeset/129483 :-)
Created attachment 165702 [details] Fix
Comment on attachment 165702 [details] Fix Attachment 165702 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14033227 New failing tests: http/tests/security/cross-frame-access-call.html
Comment on attachment 165702 [details] Fix Attachment 165702 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14002958 New failing tests: http/tests/security/xss-eval.html fast/js/eval-cross-window.html
Created attachment 165722 [details] Fixed cross-frame-access-call
Fixed in r129592
(In reply to comment #8) > Fixed in r129592 Thanks, the changes seem to work well, albeit there have been some failures on the Chromium ports: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&showAllRuns=true&tests=fast%2Fjs%2Feval-cross-window.html%2Chttp%2Ftests%2Fsecurity%2Fcross-frame-access-call.html%2Chttp%2Ftests%2Fsecurity%2Fxss-eval.html Two of these seem more like a progression though http/tests/security/xss-eval.html looks more of a failure.
(In reply to comment #9) > (In reply to comment #8) > > Fixed in r129592 > > Thanks, the changes seem to work well, albeit there have been some failures on the Chromium ports: > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&showAllRuns=true&tests=fast%2Fjs%2Feval-cross-window.html%2Chttp%2Ftests%2Fsecurity%2Fcross-frame-access-call.html%2Chttp%2Ftests%2Fsecurity%2Fxss-eval.html > > Two of these seem more like a progression though http/tests/security/xss-eval.html looks more of a failure. We don't thing the xss-eval change diminishes security. This change does not allow the parent frame to inject new code that gets to run in the navigated frame's environment - the eval takes place in the context the eval function was extracted from, which has the same security origin (and if it did not, we would not have had access to eval in the first place). The test does cover a case where the eval does try to access the new document global object, and this is correctly inhibited. cheers, G.
Unskipped on Qt - r129608.
Re-opened since this is blocked by 97670
Adding Chromium security people to verify the security assertions. We also need to know why this is causing chromium tests to fail.
> Adding Chromium security people to verify the security assertions. We also need to know why this is causing chromium tests to fail. I'm sorry, I'm not sure what you're asking. Ignoring the |this| value passed to eval sounds safe. If that's what other browsers do in this case, that seems like a good thing to do. It's certainly more useful than throwing an error if |this| doesn't match what you expect.
(In reply to comment #12) > Re-opened since this is blocked by 97670 Skipped on Qt again after the rollout - https://trac.webkit.org/changeset/129639 Please unskip it with the proper fix.
(In reply to comment #14) > > Adding Chromium security people to verify the security assertions. We also need to know why this is causing chromium tests to fail. > > I'm sorry, I'm not sure what you're asking. Ignoring the |this| value passed to eval sounds safe. If that's what other browsers do in this case, that seems like a good thing to do. It's certainly more useful than throwing an error if |this| doesn't match what you expect. Sorry, should not have used the word "assertions". I simply wanted to be sure that you agree with the analysis in comment #10, before we put this patch back in and punt the errors to the v8 team (or whomever is the right group to fix the failures).
> I simply wanted to be sure that you agree with the analysis in comment #10. Yes.
(In reply to comment #8) > Fixed in r129592 This test still seems to be failing on the Mac bots.
(In reply to comment #18) > (In reply to comment #8) > > Fixed in r129592 > > This test still seems to be failing on the Mac bots. Oh, I see the patch was rolled out. Sorry for the noise!
Hi, I didn't realize this was rolled out – relanding – please file bugs against V8 to match behaviour or skip tests as you see fit.
Relanded in r129712.
(In reply to comment #21) > Relanded in r129712. and unskipped on Qt again - r129747
I digged a little bit the history of this bug: https://trac.webkit.org/changeset/129456 - original patch https://trac.webkit.org/changeset/129460 - skip on GTK https://trac.webkit.org/changeset/129481 - skip on Qt https://trac.webkit.org/changeset/129483 - skip on EFL https://trac.webkit.org/changeset/129592 - fix landed https://trac.webkit.org/changeset/129599 - unskip on EFL https://trac.webkit.org/changeset/129608 - unskip on Qt https://trac.webkit.org/changeset/129615 - unskip on GTK https://trac.webkit.org/changeset/129629 - fix rolled out https://trac.webkit.org/changeset/129635 - skip on EFL https://trac.webkit.org/changeset/129639 - skip on Qt https://trac.webkit.org/changeset/129680 - skip on GTK https://trac.webkit.org/changeset/129712 - fix relanded https://trac.webkit.org/changeset/129740 - unskip on EFL https://trac.webkit.org/changeset/129747 - unskip on Qt ... and it it still skipped on GTK I think we should reduce the number of the commits in the future ... In my opinion if you fix a bug, you should unskip the test on all platform _with_ the proper fix. And if you rollout a patch, you should rollout its dependency too. If we had followed this suggestion, we would have spare many gardening patches. - r129592 should have contained r129599, r129608, r129615 - r129629 should have contained r129635, r129639, r129680 - r129712 should have contained r129740, r129747, r......