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.
In radar <rdar://problem/7672667>
Created attachment 49769 [details] Match Gecko's rules for for/event attributes in script tags
Attachment 49769 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/315004
Attachment 49769 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/315005
Forgot HTMLNames were autogenerated. Followup coming.
Created attachment 49773 [details] Now with properly autogenerated HTMLAttributeNames.in goodness
Attachment 49769 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/321354
GTK failure isn't my fault - they're just not picking up the new HTML names to autogenerate.
(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.
(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?
> 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.
(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.
(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.
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.)
Created attachment 49814 [details] Unchanged from last time except fixing the "Double should" typo in my layouttest
Landed in http://trac.webkit.org/changeset/55414
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.
Created attachment 49839 [details] Followup that adds whitespace stripping
Sam caught the double-stripping-of-whitespace on the for attribute and reviewed this.
http://trac.webkit.org/changeset/55428
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.