WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Brady Eidson
Comment 1
2010-03-01 17:15:48 PST
In radar <
rdar://problem/7672667
>
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
Attachment 49769
[details]
did not build on chromium: Build output:
http://webkit-commit-queue.appspot.com/results/315004
Eric Seidel (no email)
Comment 4
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
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
Attachment 49769
[details]
did not build on gtk: Build output:
http://webkit-commit-queue.appspot.com/results/321354
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
Landed in
http://trac.webkit.org/changeset/55414
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
http://trac.webkit.org/changeset/55428
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.
Top of Page
Format For Printing
XML
Clone This Bug