Bug 45693 - REGRESSION (new parser): Leopard/Tiger Mail <head>/<body> quirk is gone
Summary: REGRESSION (new parser): Leopard/Tiger Mail <head>/<body> quirk is gone
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords: InRadar
Depends on:
Blocks: 41115
  Show dependency treegraph
 
Reported: 2010-09-13 11:36 PDT by Adam Barth
Modified: 2010-10-12 17:25 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.60 KB, patch)
2010-09-13 11:39 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (10.15 KB, patch)
2010-09-13 15:47 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (9.96 KB, patch)
2010-10-12 12:47 PDT, Andy Estes
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-09-13 11:36:53 PDT
Implement Leopard/Tiger parsing quriks
Comment 1 Adam Barth 2010-09-13 11:39:56 PDT
Created attachment 67443 [details]
Patch
Comment 2 Adam Barth 2010-09-13 11:41:21 PDT
There was some talk of doing this at the WebKit layer instead.  Maybe that's better?  The parsing algorithm isn't really set up to skip over states in the state machine.
Comment 3 Eric Seidel (no email) 2010-09-13 11:42:45 PDT
Comment on attachment 67443 [details]
Patch

This is the wrong layer for this hack.  We shouldn't poop on the parser just to support some long-deprecated version of Mail.
Comment 4 Darin Adler 2010-09-13 11:51:15 PDT
(In reply to comment #3)
> This is the wrong layer for this hack.  We shouldn't poop on the parser just to support some long-deprecated version of Mail.

I don’t agree.

I’m not sure how to resolve this.

It seems the wrong process to remove the compatibility mode Apple added, and then debate how to add it back. Maybe we could have this debate before removing it next time?
Comment 5 Darin Adler 2010-09-13 11:51:50 PDT
Adding an application-specific hack to language bindings seems even worse to me.
Comment 6 Darin Adler 2010-09-13 11:53:00 PDT
Seriously, if you find a better place to put this, that’s fine, but please add it back ASAP.
Comment 7 Adam Barth 2010-09-13 12:00:00 PDT
> It seems the wrong process to remove the compatibility mode Apple added, and then debate how to add it back. Maybe we could have this debate before removing it next time?

To be clear, the patch earlier today didn't remove the compatibility mode.  That code wasn't doing what it claimed.  We inadvertently removed the mode when we originally switched to the new tree builder because there wasn't any test coverage to tell us that we broke something.
Comment 8 Darin Adler 2010-09-13 12:04:14 PDT
(In reply to comment #7)
> We inadvertently removed the mode when we originally switched to the new tree builder

Yes.

> because there wasn't any test coverage to tell us that we broke something.

I’m not going to debate you on this point. It would be great to have tests for every case; certainly make it a lot easier to rewrite a module that has thorough test coverage.

So anyway, while redoing the parser you removed this. Please add it back!

Or if you won’t add it back, let me know so I can assign engineers to undo the damage. I can’t ship WebKit until we add it back.
Comment 9 Adam Barth 2010-09-13 12:08:47 PDT
> So anyway, while redoing the parser you removed this. Please add it back!
> 
> Or if you won’t add it back, let me know so I can assign engineers to undo the damage. I can’t ship WebKit until we add it back.

I believe that path attached to this bug does add it back.  It lacks a test, but I think that's matter of figuring out how to tell DRT to turn on the quirk early enough / adjusting the #ifdefs to allow enabling the quirk in non-Tiger/Leopard builds.

If Eric thinks that it should be done at another layer, perhaps he'd be willing to post a patch showing how that would work so we can compare the two and decide which is the better approach?
Comment 10 Eric Seidel (no email) 2010-09-13 12:15:51 PDT
Could someone post a stack trace of the exception?  I don't have access to a Leopard or Tiger machine anymore.
Comment 11 Darin Adler 2010-09-13 12:21:40 PDT
5/6/09 4:54:45 PM	Mail[229]	An exception was thrown '*** NOT_FOUND_ERR: DOMException 8'. Stack trace:
(
	0   CoreFoundation                      0x00007fff88552924 __exceptionPreprocess + 180
	1   libobjc.A.dylib                     0x00007fff81eea4b7 objc_exception_throw + 45
	2   CoreFoundation                      0x00007fff88500689 -[NSException raise] + 9
	3   WebCore                             0x00007fff80681fb7 _ZN7WebCore17raiseDOMExceptionEi + 279
	4   WebCore                             0x00007fff80844ad1 -[DOMRange setEndAfter:] + 65
	5   Mail                                0x00000001001e6f03 0x0 + 4296961795
	6   Mail                                0x00000001000d3901 0x0 + 4295833857
	7   Mail                                0x00000001001e5848 0x0 + 4296955976
	8   AppKit                              0x00007fff83e08669 -[NSApplication sendAction:to:from:] + 95
	9   AppKit                              0x00007fff83e085c9 -[NSControl sendAction:to:] + 94
	10  AppKit                              0x00007fff83e28f9f -[NSCell trackMouse:inRect:ofView:untilMouseUp:] + 1715
	11  AppKit                              0x00007fff83e65dff -[NSButtonCell trackMouse:inRect:ofView:untilMouseUp:] + 555
	12  AppKit                              0x00007fff83e27a49 -[NSControl mouseDown:] + 624
	13  AppKit                              0x00007fff83dd8805 -[NSWindow sendEvent:] + 5409
	14  AppKit                              0x00007fff83d9d53a -[NSApplication sendEvent:] + 4440
	15  Mail                                0x000000010002ad02 0x0 + 4295142658
	16  AppKit                              0x00007fff83c7f6e1 -[NSApplication run] + 474
	17  AppKit                              0x00007fff83c6df6e NSApplicationMain + 364
	18  Mail                                0x00000001000e2f30 0x0 + 4295896880
)
Comment 12 Eric Seidel (no email) 2010-09-13 12:24:19 PDT
Thank you.
Comment 13 Adam Barth 2010-09-13 12:30:42 PDT
There might be a slightly cleaner approach to the parser change if we introduce a new pseudo state.  I'll try another version of the patch.
Comment 14 Eric Seidel (no email) 2010-09-13 13:58:55 PDT
@darin:  Is the content being displayed in mail at this time arbitrary HTML or is it mail-generated?

I'm looking into just moving the code into WebKit and having it simply reparent all children before the body element into the body element just before onload fires.  That should be possible to do entirely in WebKit and not involve WebCore or the parser (or other platforms) at all.
Comment 15 Eric Seidel (no email) 2010-09-13 14:12:23 PDT
Looks like implementing this as a user script injected at "document end" would be the most elegant.  I just have to figure out when to inject it into the page group and which page group to inject into. :)
Comment 16 Eric Seidel (no email) 2010-09-13 15:47:59 PDT
Created attachment 67482 [details]
Patch
Comment 17 Eric Seidel (no email) 2010-09-13 15:49:46 PDT
I've tested the quirk on my Snow Leopard machine by commenting out the "is mail" check and validating that all pages loaded have document.firstChild.firstChild == document.body.

However I can't take this to completion as I don't have a Leopard or Tiger machine to test on (not to mention access to Radar or Mail's source code).  Someone from Apple will have to carry this bird home.

The patch as-is, should solve mail's problems based on Darin's description above. :)
Comment 18 Darin Adler 2010-09-16 11:25:43 PDT
<rdar://problem/8439720>
Comment 19 Adam Barth 2010-09-16 13:31:36 PDT
Comment on attachment 67482 [details]
Patch

I'm nominating Eric's patch for review.  I'm not sure how to test it, both to make sure it works for Mail and to make sure we don't break it in the future...
Comment 20 Adam Barth 2010-09-26 22:32:35 PDT
@darin, should someone review this patch so we can make progress on this issue?  This is one of the few remaining issues with the new parser.  I'd like to clean them out and be done with this project.  Thanks.
Comment 21 Andy Estes 2010-09-27 16:40:23 PDT
(In reply to comment #20)
> @darin, should someone review this patch so we can make progress on this issue?  This is one of the few remaining issues with the new parser.  I'd like to clean them out and be done with this project.  Thanks.

I have a Leopard machine and can test this patch. Will report back shortly.
Comment 22 Andy Estes 2010-09-27 21:36:56 PDT
(In reply to comment #19)
> (From update of attachment 67482 [details])
> I'm nominating Eric's patch for review.  I'm not sure how to test it, both to make sure it works for Mail and to make sure we don't break it in the future...

I'm still seeing the NOT_FOUND_ERR logged to Console. I did verify in gdb that the script was being injected, but perhaps it's not being executed by the time Mail does its DOM operations. I'll spend a little time investigating.

This patch will also need a layout test and the DRT machinery necessary to enable the quirk.
Comment 23 Andy Estes 2010-10-12 12:47:35 PDT
Created attachment 70557 [details]
Patch
Comment 24 Andy Estes 2010-10-12 12:51:06 PDT
I just got a chance to look at this again and figured out why it wasn't working. Mail doesn't specify a group name when initializing its WebViews, so calling _addUserScriptToGroup doesn't work in this case. I changed the patch to get the single-page page group for the WebView and call PageGroup::addUserScriptToWorld() directly, which works.
Comment 25 Adam Barth 2010-10-12 13:16:11 PDT
> ..., which works.

Cool!  Shall we go with this approach then?
Comment 26 Andy Estes 2010-10-12 13:49:44 PDT
(In reply to comment #25)
> > ..., which works.
> 
> Cool!  Shall we go with this approach then?

Could there be issues with applying this quirk to arbitrary HTML email content? I assume things that typically go in head retain their meaning when head is a child of body rather than html.

Also, this quirk seems to only be needed by the WebView created for the signatures pref panel, so it would be nice if this could be isolated to that WebView only.(In reply to comment #25)
> > ..., which works.
> 
> Cool!  Shall we go with this approach then?

I wish there was a way to isolate this quirk to the signature pref pane's WebView. It seems strange to be reparenting the head element in arbitrary HTML email.
Comment 27 Adam Barth 2010-10-12 13:53:27 PDT
> Could there be issues with applying this quirk to arbitrary HTML email content? I assume things that typically go in head retain their meaning when head is a child of body rather than html.

In theory, yes, but in practice, it's probably fine.  One thing to check is that scripts don't get executed twice if moved in this way.  (They shouldn't, but one should check these things.)

> I wish there was a way to isolate this quirk to the signature pref pane's WebView. It seems strange to be reparenting the head element in arbitrary HTML email.

Is there some distinguishing characteristic of that WebView we can key off of?  Maybe it's URL?  Maybe something about the content?
Comment 28 Andy Estes 2010-10-12 13:55:09 PDT
(In reply to comment #27)
> > Could there be issues with applying this quirk to arbitrary HTML email content? I assume things that typically go in head retain their meaning when head is a child of body rather than html.
> 
> In theory, yes, but in practice, it's probably fine.  One thing to check is that scripts don't get executed twice if moved in this way.  (They shouldn't, but one should check these things.)

I believe Mail disables scripting.

> 
> > I wish there was a way to isolate this quirk to the signature pref pane's WebView. It seems strange to be reparenting the head element in arbitrary HTML email.
> 
> Is there some distinguishing characteristic of that WebView we can key off of?  Maybe it's URL?  Maybe something about the content?

Perhaps; I'll take a look in the debugger.
Comment 29 Andy Estes 2010-10-12 15:53:55 PDT
> Is there some distinguishing characteristic of that WebView we can key off of?  Maybe it's URL?  Maybe something about the content?

The only distinguishing characteristic I can come up with is that the WebView is decoded from a NIB file (initWithCoder) as opposed to email WebViews (initWithFrame). The WebView doesn't have a frame name, group name, or main document URL, so I'm not sure what else to key off of.
Comment 30 Eric Seidel (no email) 2010-10-12 16:05:58 PDT
Comment on attachment 70557 [details]
Patch

I think the risk of moving the <head> into the <body> of all emails in Leopard/Tiger versions of mail, is fantastically small.

But I may also be used to the chromium update-every-few-days model where regressions are easier to fix when you get them wrong.

I'm ready to r+ this patch, but I shouldn't really... since I wrote it. :)
Comment 31 Darin Adler 2010-10-12 16:40:52 PDT
Comment on attachment 70557 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70557&action=review

r=me but I’d like to see the style issues fixed

> WebKit/mac/WebView/WebView.mm:648
> ++ (NSString*)_mailQuirksUserScript
> +{
> +    static NSString* mailQuirksScript = nil;
> +    if (!mailQuirksScript) {
> +        NSString *scriptPath = [[NSBundle bundleForClass:[WebView class]] pathForResource:@"MailQuirksUserScript" ofType:@"js"];
> +        mailQuirksScript = [[NSString alloc] initWithContentsOfFile:scriptPath];
> +    }
> +    return mailQuirksScript;
> +}

The one-time initialization code should not use an if statement. Instead it should be in a separate function.

> WebKit/mac/WebView/WebView.mm:658
> +    core(self)->group().addUserScriptToWorld(core([WebScriptWorld world]),
> +                                             [WebView _mailQuirksUserScript],
> +                                             KURL(),
> +                                             0,
> +                                             0,
> +                                             InjectAtDocumentEnd,
> +                                             InjectInAllFrames);

We don’t line up code with parentheses like this. This should be indented normally and not formatted vertically like this.
Comment 32 Andy Estes 2010-10-12 16:45:09 PDT
Thanks for the review Darin. I'll clean up the style issues before landing.
Comment 33 Andy Estes 2010-10-12 17:25:30 PDT
Committed r69622: <http://trac.webkit.org/changeset/69622>