Bug 35556 - REGRESSION(r51097) - Unable to log in to statefarm.com
Summary: REGRESSION(r51097) - Unable to log in to statefarm.com
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2010-03-01 17:15 PST by Brady Eidson
Modified: 2010-03-02 13:47 PST (History)
6 users (show)

See Also:


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-
Details | Formatted Diff | Diff
Now with properly autogenerated HTMLAttributeNames.in goodness (7.50 KB, patch)
2010-03-01 18:53 PST, Brady Eidson
beidson: commit-queue-
Details | Formatted Diff | Diff
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-
Details | Formatted Diff | Diff
Followup that adds whitespace stripping (5.23 KB, patch)
2010-03-02 13:38 PST, Brady Eidson
sam: review+
beidson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 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.
Comment 1 Brady Eidson 2010-03-01 17:15:48 PST
In radar <rdar://problem/7672667>
Comment 2 Brady Eidson 2010-03-01 18:37:23 PST
Created attachment 49769 [details]
Match Gecko's rules for for/event attributes in script tags
Comment 3 WebKit Review Bot 2010-03-01 18:44:25 PST
Attachment 49769 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/315004
Comment 4 Eric Seidel (no email) 2010-03-01 18:44:46 PST
Attachment 49769 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/315005
Comment 5 Brady Eidson 2010-03-01 18:51:13 PST
Forgot HTMLNames were autogenerated.  Followup coming.
Comment 6 Brady Eidson 2010-03-01 18:53:52 PST
Created attachment 49773 [details]
Now with properly autogenerated HTMLAttributeNames.in goodness
Comment 7 WebKit Review Bot 2010-03-01 19:01:22 PST
Attachment 49769 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/321354
Comment 8 Brady Eidson 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.
Comment 9 Adam Barth 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.
Comment 10 Brady Eidson 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?
Comment 11 Adam Barth 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.
Comment 12 Brady Eidson 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.
Comment 13 Brady Eidson 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.
Comment 14 Adam Barth 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.)
Comment 15 Brady Eidson 2010-03-02 10:14:48 PST
Created attachment 49814 [details]
Unchanged from last time except fixing the "Double should" typo in my layouttest
Comment 16 Brady Eidson 2010-03-02 10:35:13 PST
Landed in http://trac.webkit.org/changeset/55414
Comment 17 Brady Eidson 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.
Comment 18 Brady Eidson 2010-03-02 13:38:11 PST
Created attachment 49839 [details]
Followup that adds whitespace stripping
Comment 19 Brady Eidson 2010-03-02 13:45:38 PST
Sam caught the double-stripping-of-whitespace on the for attribute and reviewed this.
Comment 20 Brady Eidson 2010-03-02 13:46:42 PST
http://trac.webkit.org/changeset/55428
Comment 21 Sam Weinig 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.