Bug 145472 – NavigationAction constructor cleanup

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 253916 [details]
More red than green.

[Alex Christensen]

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.

[Brady Eidson]

http://trac.webkit.org/changeset/185007

[Alexey Proskuryakov]

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?

[Brady Eidson]

(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...?

[Brady Eidson]

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...

[Brady Eidson]

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?

[Brady Eidson]

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.

[Brady Eidson]

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

[Brady Eidson]

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.

[Brady Eidson]

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.

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 253965 [details]
Alternate fix that should definitely work.

[Brady Eidson]

Reopening for commit queue to work

[Brady Eidson]

Created attachment 253966 [details]
Fix for landing

[Brady Eidson]

NM, stupid commit queue...

[Brady Eidson]

http://trac.webkit.org/changeset/185033

[Brady Eidson]

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.

[Alexey Proskuryakov]

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);

[Alexey Proskuryakov]

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.

[Brady Eidson]

(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!