NavigationAction constructor cleanup Lot's of NavigationAction constructors, all subtley different. An upcoming patch I'm working on will add another one. Seemed like a good time to re-route them all to a central constructor via c'tor delegation.
Created attachment 253916 [details] More red than green.
Comment on attachment 253916 [details] More red than green. View in context: https://bugs.webkit.org/attachment.cgi?id=253916&action=review > Source/WebCore/loader/NavigationAction.h:47 > + NavigationAction(const ResourceRequest&, NavigationType, PassRefPtr<Event>, ShouldOpenExternalURLsPolicy); The new constructor could be private right now, but it might need to be public in a following patch. Ok either way.
http://trac.webkit.org/changeset/185007
This broke four tests on Windows: <https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r185031%20(52131)/results.html>. Brady, are you available to investigate today?
(In reply to comment #4) > This broke four tests on Windows: > <https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/ > r185031%20(52131)/results.html>. > > Brady, are you available to investigate today? ...WHAT...?
Okay, without actually being able to reproduce because I can't get to a windows setup right now... 4 tests all fail because NavigationType::LinkClicked became NavigationType::Other. Why on Windows and not Mac? Who knows. The only way this patch could've made LinkClicked become Other is if: static NavigationType navigationType(FrameLoadType frameLoadType, bool isFormSubmission, bool haveEvent) ... is giving different results. The only way it gives LinkClicked is if "haveEvent" is true. So previously a called had an event, and now they don't. Will explore...
Only two of the ctor's called navigationType(). One was navigationType(frameLoadType, isFormSubmission, 0) (and is identical after the patch), and that call never could've given LinkClicked. So it must be the other, which passes a PassRefPtr<Event>: navigationType(frameLoadType, isFormSubmission, event) It's identical afterwards, too. But maybe there's some weird PassRefPtr automatic assignment stuff that's killing the event ptr?
Are constructor arguments evaluated in reverse, right to left? If so, the call to: + : NavigationAction(resourceRequest, navigationType(frameLoadType, isFormSubmission, event), event, ShouldOpenExternalURLsPolicy::ShouldNotAllow) ...might null-out the PassRefPtr for "event" before passing it into navigationType() as a bool argument...? That's really my only guess here.
Holy crap, I think that's totally it. C++ spec says arguments in the order they appear, but non-clang compilers have gotten it wrong. According to http://stackoverflow.com/questions/14060264/order-of-evaluation-of-elements-in-list-initialization gcc had it wrong for awhile... And visual studio still does in the context of initializer lists: https://connect.microsoft.com/VisualStudio/feedbackdetail/view/976911/braced-initializer-list-not-evaluated-left-to-right I'm guessing the VS bug is not strictly limited to initializer lists? Unless there's something obvious that somebody else is seeing that I'm not, I think that might be it. o_O
Actually, maybe this is bogus for all C++... The C++11 standard demands in §8.5.4/4 that the items in a braced initializer list are evaluated left-to-right: "Within the initializer-list of a braced-init-list, the initializer-clauses, including any that result from pack expansions (14.5.3), are evaluated in the order in which they appear. That is, every value computation and side effect associated with a given initializer-clause is sequenced before every value computation and side effect associated with any initializer-clause that follows it in the comma-separated list of the initializer-list. [Note: This evaluation ordering holds regardless of the semantics of the initialization; for example, it applies when the elements of the initializer-list are interpreted as arguments of a constructor call, even though ordinarily there are no sequencing constraints on the arguments of a call. — end note]" The left-to-right evaluation sequencing is required for braced initialization lists, but not for "normal" constructor calls - "ordinarily there are no sequencing constraints on the arguments of a call." So I wonder if clang just does LTR, but it's not required to be LTR. Maybe VS isn't "wrong" here, even if they are weird.
Created attachment 253962 [details] Proposed windows test fix This is my proposed fix that I'm building on Mac right now. Pros - It's not a dirty hack to work around a possible "weirdness" in the VS C++ compiler, it's cross platform, it simply turns these into braced initializer lists which the C++ spec requires to be evaluated left-to-right Cons - If there *IS* a VS bug in evaluating initializer lists LTR... this still won't fix it.
I'm not even going to mess with the initializer list version of this. In reality, nobody should actually be passing ownership of the event, so a PassRefPtr makes no sense. Raw ptrs make much more sense, and won't have this issue.
Created attachment 253965 [details] Alternate fix that should definitely work.
Reopening for commit queue to work
Created attachment 253966 [details] Fix for landing
NM, stupid commit queue...
http://trac.webkit.org/changeset/185033
Just for posterity, the even crazier thing was VS had this behavior in release builds, but not debug - the debug tests never broke! Sigh All fixed.
Nice fix! We should eliminate PassRefPtr<Event> from the callers too, we have atrocities there like frame->loader().urlSelected(url, target(), PassRefPtr<Event>(&event), LockHistory::No, LockBackForwardList::No, MaybeSendReferrer);
It is not very surprising that the failure was release only - evaluation reordering is an optimization. I wouldn't be too surprised if clang decided to do the same in some cases.
(In reply to comment #19) > Nice fix! > > We should eliminate PassRefPtr<Event> from the callers too, we have > atrocities there like > > frame->loader().urlSelected(url, target(), PassRefPtr<Event>(&event), > LockHistory::No, LockBackForwardList::No, MaybeSendReferrer); Yup, definitely noticed that while deciding on the right fix, but it was out of scope from a test fix. I will followup some point soon!