Bug 7445 - REGRESSION: Gmail puts wrong subject in replies
Summary: REGRESSION: Gmail puts wrong subject in replies
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: GoogleBug, HasReduction, InRadar, Regression
: 7253 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-02-24 12:28 PST by Diego Miguel Virasoro
Modified: 2006-11-03 02:02 PST (History)
4 users (show)

See Also:


Attachments
screenshot (55.10 KB, image/png)
2006-07-05 12:20 PDT, Alexey Proskuryakov
no flags Details
Reduction for Gmail Re: problem (2.25 KB, text/html)
2006-10-25 04:38 PDT, Wesley Moore
no flags Details
Proposed patch for this bug (2.14 KB, patch)
2006-10-26 13:25 PDT, W. Andy Carrel
no flags Details | Formatted Diff | Diff
A better patch for this bug (5.17 KB, patch)
2006-10-26 14:34 PDT, W. Andy Carrel
no flags Details | Formatted Diff | Diff
Patch with ChangeLog edit (4.37 KB, patch)
2006-10-26 21:35 PDT, W. Andy Carrel
no flags Details | Formatted Diff | Diff
automated test (3.70 KB, patch)
2006-10-27 12:17 PDT, Alexey Proskuryakov
mjs: review+
Details | Formatted Diff | Diff
Tests for Unicode RegExp behavior to match IE and Firefox (8.41 KB, patch)
2006-10-27 20:55 PDT, W. Andy Carrel
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Diego Miguel Virasoro 2006-02-24 12:28:40 PST
(note: I am not sure if this is a problem with webkit or google, but it does work fine in Safari 2.0.3, whereas I have problems with the Webkit from 23rd Feb)

When you click reply to a message in Gmail, it adds "Re: " to the subject, even if there is one already. A short conversation can end up with a subject like "Re: Re: Re: Re: Re: subject here".

Now I know this is a really minor quirk but it does work with Safari 2.0.3 so I guess it may be the latest webkit misbehaving somewhere, and that may have more serious results.

Note that because Gmail structures your emails in threads, you need to activly check the subject: eg. after clicking on reply, click on "Edit subject" which will show the subject field. There you can see the current subject.
Comment 1 Gustaaf Groenendaal (MysteryQuest) 2006-06-27 15:57:27 PDT
I can confirm this as a bug. Google helped out on this one and said it is not the expected behaviour.
Comment 2 Gustaaf Groenendaal (MysteryQuest) 2006-06-27 16:06:08 PDT
Some additional information: ToT doesn't seem to like XmlHTTPRequest() like Google's with a redirect
Comment 3 Alexey Proskuryakov 2006-06-27 22:10:31 PDT
Marking as a regression.

Gustaaf, could you please explain how this relates to XMLHttpRequest? It doesn't look obvious from the description to me, so that would save some time re-doing your investigation.
Comment 4 Alice Liu 2006-07-05 08:34:03 PDT
Is this bug happening for anyone else?  I am not experiencing this at gmail.  Marking as worksforme now.  Please change it back if anyone can reproduce the problem.  
Comment 5 Diego Miguel Virasoro 2006-07-05 09:52:45 PDT
I just want to add that at least for me (the reporter) using the latest webkit (5th july) this still occurs.

(I am using OS 10.4.7 and have no Safari extensions installed)
Comment 6 Alexey Proskuryakov 2006-07-05 12:19:44 PDT
Yes, I also can reproduce. Reopening.
Comment 7 Alexey Proskuryakov 2006-07-05 12:20:35 PDT
Created attachment 9212 [details]
screenshot
Comment 8 Alice Liu 2006-07-05 13:58:15 PDT
<rdar://problem/4614195>
Comment 9 David Kilzer (:ddkilzer) 2006-07-12 21:58:18 PDT
GMail is Google.

Comment 10 Wesley Moore 2006-10-25 04:38:59 PDT
Created attachment 11203 [details]
Reduction for Gmail Re: problem

After some JavaScript debugging with Drosera and Venkman I've reduced this bug the main culprit, which appears to be Unicode escape sequences in a regular expression. The reduction is attached, the function names and variables mostly came from the obfuscated Gmail source which is why they are weird.
Comment 11 Alexey Proskuryakov 2006-10-25 07:57:06 PDT
Good detective job!

BTW, the regression is already in shipping WebKit as of 10.4.8.
Comment 12 W. Andy Carrel 2006-10-26 13:25:29 PDT
Created attachment 11228 [details]
Proposed patch for this bug

I strongly suspect this bug is an affect of the issue in 7253. Namely that inline regexes (like /\u2620/i) match nothing (not even "\\u2620") while explicitly created regexes (like new RegExp("\u2620","i")) will match the "skull and crossbones" character.

I have a patch put together that corrects for this bug (see attachment). I've run the patch through the JavaScriptCore tests and used (slightly modified) Wesley's reduction in testkjs to make sure it corrects the behavior for this case.

There is a new failure in the kjs tests as a result of the patch:
ecma_2/RegExp/properties-001.js
--> /\Q1/im.source = \Q1 FAILED! expected: \u0051
--> /\Q1/im.toString() = /\Q1/im FAILED! expected: /\u0051/im

I believe this is more a sign that RegExp objects need to be unicode escaping their output than the patch is doing the wrong thing. But I also haven't looked at the specification to see exactly what should/shouldn't be returned for .source and .toString(), so I could be wrong about that.
Comment 13 W. Andy Carrel 2006-10-26 14:34:04 PDT
Created attachment 11229 [details]
A better patch for this bug

So here's another whack at this that should work better. I had forgotten to block the '\' in '\ua9f4' from getting collected and the shift() was one short of eating all the hex bytes. So, now these are all fixed. 

There is also a change to the ecma_2/RegExp/properties-001.js case for unicode. some_regex.source and some_regex.toString() do not have to match the original source code according to the ECMA spec at 15.10.6.4. The key is that the expression can be passed back into 'new RegExp(some_regex.source, FLAGS)' and 'eval(some_regex.toString())' and still match the same things which it does in this case.
Comment 14 Alexey Proskuryakov 2006-10-26 21:31:43 PDT
I'm wondering why this is a regression...

Please mark this patch for review (assuming you intend it to be reviewed), by clicking on Edit link and selecting '?' from a popup there. You may want to include patches to ChangeLogs explaining the changes (we have a prepare-ChangeLog script that helps with this), and the fix certainly needs an automated test case - see <http://webkit.org/coding/contributing.html>.
Comment 15 W. Andy Carrel 2006-10-26 21:35:10 PDT
Created attachment 11232 [details]
Patch with ChangeLog edit

Now with ChangeLog goodness.
Comment 16 Maciej Stachowiak 2006-10-26 22:07:20 PDT
Comment on attachment 11232 [details]
Patch with ChangeLog edit

r=me
Comment 17 Mark Rowe (bdash) 2006-10-26 22:37:24 PDT
Landed in r17354.
Comment 18 Alexey Proskuryakov 2006-10-27 03:31:40 PDT
I still think this definitely needs a test case; I'll make one.
Comment 19 Alexey Proskuryakov 2006-10-27 03:33:17 PDT
Comment on attachment 11232 [details]
Patch with ChangeLog edit

Clearing review flag from the landed patch, so that it doesn't appear in commit queue.
Comment 20 W. Andy Carrel 2006-10-27 08:16:10 PDT
(In reply to comment #18)
> I still think this definitely needs a test case; I'll make one.
> 

Both the reduction from this or 7253 would make good test cases. There should probably also be test cases that capture some Unicode character or other.

The existing test case that the landed patch modified should also fail if the unicode escape handling code is reverted for some reason.
Comment 21 W. Andy Carrel 2006-10-27 08:25:41 PDT
Confirmed correct subject line behavior on Gmail and Google Apps For Your Domain ("hosted Gmail") with latest build.
Comment 22 David Kilzer (:ddkilzer) 2006-10-27 09:49:33 PDT
*** Bug 7253 has been marked as a duplicate of this bug. ***
Comment 23 Alexey Proskuryakov 2006-10-27 12:17:27 PDT
Created attachment 11255 [details]
automated test
Comment 24 Darin Adler 2006-10-27 15:57:51 PDT
I believe this fix may be incorrect because if you use the \u syntax for metacharacters, they'll have their metacharacter effect. Ideally we'd have some tests for this. For example, does \u007c match a | or does it work as a | in a regexp would.

Also, the fact that when you serialize the regular expression, the \u sequence is decoded, and that's not Firefox's behavior. That shows up in the test results for fast/js/kde/RegExp.html.

We might need to write bug reports for one or both of those issues and possibly do a new fix for this.
Comment 25 W. Andy Carrel 2006-10-27 16:25:06 PDT
(In reply to comment #24)
> I believe this fix may be incorrect because if you use the \u syntax for
> metacharacters, they'll have their metacharacter effect. Ideally we'd have some
> tests for this. For example, does \u007c match a | or does it work as a | in a
> regexp would.

Is the \u syntax guaranteed to escape metacharacters? If so, a workaround might be to embed the character into a character class with appropriate escaping if needed. The ECMA spec is silent on this issue and the current behavior seems desirable insomuch as it causes /a\u007cb/ do the same thing as new RegExp("a\u007cb"), where the \u007c is translated into | by the string processing code before RegExp() gets to see it.

> Also, the fact that when you serialize the regular expression, the \u sequence
> is decoded, and that's not Firefox's behavior. That shows up in the test
> results for fast/js/kde/RegExp.html.

This is a valid behavior according to the spec. The rule is that the expression can be fed back in to new RegExp() and still match the same things. OTOH, if people are counting on the escaping behavior, we'll want to set aside the exact string that was the input as it is read and present that when .source or .toString() are called. Alternatively all Unicode could be re-escaped on the way out of .source and .toString().

> We might need to write bug reports for one or both of those issues and possibly
> do a new fix for this.
Comment 26 W. Andy Carrel 2006-10-27 16:59:56 PDT
I'm going to put together some more explicit tests about all the border conditions about representing unicode in regexps tonight and test them this weekend with Firefox and MSIE and will report back with the results (and a copy of the js used for the tests).

Some evidence should give a stronger hint of which way to go.
Comment 27 W. Andy Carrel 2006-10-27 20:55:00 PDT
Created attachment 11264 [details]
Tests for Unicode RegExp behavior to match IE and Firefox

These tests have passed fully with IE 7 and Firefox 1.5/2.0 (and a recent Camino nightly). They will fail at first here in WebKit, hopefully we can fix that.
Comment 28 Maciej Stachowiak 2006-10-29 18:05:23 PST
Comment on attachment 11264 [details]
Tests for Unicode RegExp behavior to match IE and Firefox

r=me

Good to land this test even if we don't pass all the tests yet. We should also file a follow-up bug on the ones we currently fail.
Comment 29 Anders Carlsson 2006-11-02 19:30:35 PST
I've committed these but I'm going to let the bug stay open since I don't think it has been fixed. Please close if this isn't true.

Committed revision 17556.
Comment 30 Alexey Proskuryakov 2006-11-03 02:02:38 PST
Filed bug 11501 and bug 11502.