RESOLVED FIXED 35556
REGRESSION(r51097) - Unable to log in to statefarm.com
https://bugs.webkit.org/show_bug.cgi?id=35556
Summary REGRESSION(r51097) - Unable to log in to statefarm.com
Brady Eidson
Reported 2010-03-01 17:15:00 PST
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.
Attachments
Match Gecko's rules for for/event attributes in script tags (7.13 KB, patch)
2010-03-01 18:37 PST, Brady Eidson
beidson: commit-queue-
Now with properly autogenerated HTMLAttributeNames.in goodness (7.50 KB, patch)
2010-03-01 18:53 PST, Brady Eidson
beidson: commit-queue-
Unchanged from last time except fixing the "Double should" typo in my layouttest (7.50 KB, patch)
2010-03-02 10:14 PST, Brady Eidson
sam: review+
beidson: commit-queue-
Followup that adds whitespace stripping (5.23 KB, patch)
2010-03-02 13:38 PST, Brady Eidson
sam: review+
beidson: commit-queue-
Brady Eidson
Comment 1 2010-03-01 17:15:48 PST
Brady Eidson
Comment 2 2010-03-01 18:37:23 PST
Created attachment 49769 [details] Match Gecko's rules for for/event attributes in script tags
WebKit Review Bot
Comment 3 2010-03-01 18:44:25 PST
Eric Seidel (no email)
Comment 4 2010-03-01 18:44:46 PST
Brady Eidson
Comment 5 2010-03-01 18:51:13 PST
Forgot HTMLNames were autogenerated. Followup coming.
Brady Eidson
Comment 6 2010-03-01 18:53:52 PST
Created attachment 49773 [details] Now with properly autogenerated HTMLAttributeNames.in goodness
WebKit Review Bot
Comment 7 2010-03-01 19:01:22 PST
Brady Eidson
Comment 8 2010-03-01 19:15:44 PST
GTK failure isn't my fault - they're just not picking up the new HTML names to autogenerate.
Adam Barth
Comment 9 2010-03-01 21:33:12 PST
(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
Comment 10 2010-03-01 22:20:13 PST
(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
Comment 11 2010-03-01 22:26:38 PST
> 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
Comment 12 2010-03-01 22:56:19 PST
(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
Comment 13 2010-03-01 22:59:14 PST
(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
Comment 14 2010-03-01 23:03:19 PST
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
Comment 15 2010-03-02 10:14:48 PST
Created attachment 49814 [details] Unchanged from last time except fixing the "Double should" typo in my layouttest
Brady Eidson
Comment 16 2010-03-02 10:35:13 PST
Brady Eidson
Comment 17 2010-03-02 13:33:12 PST
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
Comment 18 2010-03-02 13:38:11 PST
Created attachment 49839 [details] Followup that adds whitespace stripping
Brady Eidson
Comment 19 2010-03-02 13:45:38 PST
Sam caught the double-stripping-of-whitespace on the for attribute and reviewed this.
Brady Eidson
Comment 20 2010-03-02 13:46:42 PST
Sam Weinig
Comment 21 2010-03-02 13:47:59 PST
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.
Note You need to log in before you can comment on or make changes to this bug.