Bug 7445 – REGRESSION: Gmail puts wrong subject in replies

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Diego Miguel Virasoro]

(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.

[Gustaaf Groenendaal (MysteryQuest)]

I can confirm this as a bug. Google helped out on this one and said it is not the expected behaviour.

[Gustaaf Groenendaal (MysteryQuest)]

Some additional information: ToT doesn't seem to like XmlHTTPRequest() like Google's with a redirect

[Alexey Proskuryakov]

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.

[Alice Liu]

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.  

[Diego Miguel Virasoro]

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)

[Alexey Proskuryakov]

Yes, I also can reproduce. Reopening.

[Alexey Proskuryakov]

Created attachment 9212 [details]
screenshot

[Alice Liu]

<rdar://problem/4614195>

[David Kilzer (:ddkilzer)]

GMail is Google.

[Wesley Moore]

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.

[Alexey Proskuryakov]

Good detective job!

BTW, the regression is already in shipping WebKit as of 10.4.8.

[W. Andy Carrel]

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.

[W. Andy Carrel]

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.

[Alexey Proskuryakov]

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>.

[W. Andy Carrel]

Created attachment 11232 [details]
Patch with ChangeLog edit

Now with ChangeLog goodness.

[Maciej Stachowiak]

Comment on attachment 11232 [details]
Patch with ChangeLog edit

r=me

[Mark Rowe (bdash)]

Landed in r17354.

[Alexey Proskuryakov]

I still think this definitely needs a test case; I'll make one.

[Alexey Proskuryakov]

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.

[W. Andy Carrel]

(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.

[W. Andy Carrel]

Confirmed correct subject line behavior on Gmail and Google Apps For Your Domain ("hosted Gmail") with latest build.

[David Kilzer (:ddkilzer)]

*** Bug 7253 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

Created attachment 11255 [details]
automated test

[Darin Adler]

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.

[W. Andy Carrel]

(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.

[W. Andy Carrel]

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.

[W. Andy Carrel]

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.

[Maciej Stachowiak]

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.

[Anders Carlsson]

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.

[Alexey Proskuryakov]

Filed bug 11501 and bug 11502.