Bug 35556 – REGRESSION(r51097) - Unable to log in to statefarm.com

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Brady Eidson]

REGRESSION(r51097) - Unable to log in to statefarm.com

Logging in to statefarm.com takes you to a page with the following content:

<script EVENT="onload()" FOR="window" LANGUAGE="JavaScript">
... /* some cookie stuff */ ...
   document.location.replace("https://online.statefarm.com/apps/SecurityQA/ChallengeQA.asp?returnURL=http://www.statefarm.com/account.htm");
//-->
</script>

After r51097, we wouldn't execute any script with a "for" attribute.

Firefox handles this fine.  They have a rule that allows:
-Only "for"
-Only "event"
-"for" and "event" where "for=window" and "event=onload()"

See https://bugzilla.mozilla.org/show_bug.cgi?id=184159 for more discussion about this.

We should copy their rule.

[Brady Eidson]

In radar <rdar://problem/7672667>

[Brady Eidson]

Created attachment 49769 [details]
Match Gecko's rules for for/event attributes in script tags

[WebKit Review Bot]

Attachment 49769 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/315004

[Eric Seidel (no email)]

Attachment 49769 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/315005

[Brady Eidson]

Forgot HTMLNames were autogenerated.  Followup coming.

[Brady Eidson]

Created attachment 49773 [details]
Now with properly autogenerated HTMLAttributeNames.in goodness

[WebKit Review Bot]

Attachment 49769 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/321354

[Brady Eidson]

GTK failure isn't my fault - they're just not picking up the new HTML names to autogenerate.

[Adam Barth]

(In reply to comment #8)
> GTK failure isn't my fault - they're just not picking up the new HTML names to
> autogenerate.

There was a time lag.  The GTK build is pretty slow, so it was still complaining about your old patch.  The new one builds just fine on GTK.

[Brady Eidson]

(In reply to comment #9)
> (In reply to comment #8)
> > GTK failure isn't my fault - they're just not picking up the new HTML names to
> > autogenerate.
> 
> There was a time lag.  The GTK build is pretty slow, so it was still
> complaining about your old patch.  The new one builds just fine on GTK.

Why didn't the build system attribute the failure to the old build?  Why didn't it know it hadn't built the new patch yet?

[Adam Barth]

> Why didn't the build system attribute the failure to the old build?  Why didn't
> it know it hadn't built the new patch yet?

I'm not sure I understand your question.  Your first attachment (49769) didn't build on Chromium, Mac, or Gtk.  The Gtk bot was slow, so it didn't finish running until after you uploaded the second attachment (49773).  Later on, all the bots ran on your new patch and they all passed.  The bots could check whether the patch is obsolete before posting failure messages about the patch, but they aren't smart enough to do that yet.

[Brady Eidson]

(In reply to comment #11)
> > Why didn't the build system attribute the failure to the old build?  Why didn't
> > it know it hadn't built the new patch yet?
> 
> I'm not sure I understand your question. 
> ...
> The bots could check
> whether the patch is obsolete before posting failure messages about the patch,
> but they aren't smart enough to do that yet.

Yes, you do understand my question  ;)

The tool gave me a false negative.  I understand your explanation of *why* it did, but... it did.  That was the crux of my question.

[Brady Eidson]

(In reply to comment #12)
> (In reply to comment #11)
> > > Why didn't the build system attribute the failure to the old build?  Why didn't
> > > it know it hadn't built the new patch yet?
> > 
> > I'm not sure I understand your question. 
> > ...
> > The bots could check
> > whether the patch is obsolete before posting failure messages about the patch,
> > but they aren't smart enough to do that yet.
> 
> Yes, you do understand my question  ;)
> 
> The tool gave me a false negative.  I understand your explanation of *why* it
> did, but... it did.  That was the crux of my question.

Upon further reflection, I see that the bot does mention which attachment it was that caused the failure.

That is nice, but the attachment ID's aren't even on the bugzilla page.  At a glance, I'm not likely to cross reference an attachment id with a particular patch when the id isn't otherwise visible on the page.

Possible solutions:
- Posting the attachment title in the failure message
- Specifically saying "obsolete patch" or "not the most recent patch" or some other text to that effect.

[Adam Barth]

The easiest thing is probably to not spam the bug when the attachment is obsolete.  The bubbles will still have the right colors.  (I actually think the bubbles are way more useful than the comments.)

[Brady Eidson]

Created attachment 49814 [details]
Unchanged from last time except fixing the "Double should" typo in my layouttest

[Brady Eidson]

Landed in http://trac.webkit.org/changeset/55414

[Brady Eidson]

Darin pointed out a difference between Firefox's rule that I missed: they strip whitespace before checking window/onload/onload()

Followup patch on its way.

[Brady Eidson]

Created attachment 49839 [details]
Followup that adds whitespace stripping

[Brady Eidson]

Sam caught the double-stripping-of-whitespace on the for attribute and reviewed this.

[Brady Eidson]

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

[Sam Weinig]

Comment on attachment 49839 [details]
Followup that adds whitespace stripping

>      
> -    return equalIgnoringCase(forAttribute, "window") && (equalIgnoringCase(eventAttribute, "onload") || equalIgnoringCase(eventAttribute, "onload()"));
> +    forAttribute = forAttribute.stripWhiteSpace();
> +    eventAttribute = eventAttribute.stripWhiteSpace();
> +    return equalIgnoringCase(forAttribute.stripWhiteSpace(), "window") && (equalIgnoringCase(eventAttribute, "onload") || equalIgnoringCase(eventAttribute, "onload()"));

Double whitespace stripping.  Otherwise, this looks fine.  We should really add functions to do equality ignoring leading/trailing whitespace.

r=me with out the redundancy.