Bug 45693

Summary:

REGRESSION (new parser): Leopard/Tiger Mail <head>/<body> quirk is gone

Product:

WebKit

Reporter:

Adam Barth <abarth>

Component:

New Bugs

Assignee:

Adam Barth <abarth>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aa, aestes, ap, darin, eric

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Bug Depends on:

Bug Blocks:

41115

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	darin: review+

Adam Barth

Reported 2010-09-13 11:36:53 PDT

Implement Leopard/Tiger parsing quriks

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Adam Barth

Comment 1 2010-09-13 11:39:56 PDT

Created attachment 67443 [details] Patch

Adam Barth

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

Eric Seidel (no email)

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

Darin Adler

Comment 4 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?

Darin Adler

Comment 5 2010-09-13 11:51:50 PDT

Adding an application-specific hack to language bindings seems even worse to me.

Darin Adler

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

Adam Barth

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

Darin Adler

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

Adam Barth

Comment 9 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?

Eric Seidel (no email)

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

Darin Adler

Comment 11 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 )

Eric Seidel (no email)

Comment 12 2010-09-13 12:24:19 PDT

Thank you.

Adam Barth

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

Eric Seidel (no email)

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

Eric Seidel (no email)

Comment 15 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. :)

Eric Seidel (no email)

Comment 16 2010-09-13 15:47:59 PDT

Created attachment 67482 [details] Patch

Eric Seidel (no email)

Comment 17 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. :)

Darin Adler

Comment 18 2010-09-16 11:25:43 PDT

<rdar://problem/8439720>

Adam Barth

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

Adam Barth

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

Andy Estes

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

Andy Estes

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

Andy Estes

Comment 23 2010-10-12 12:47:35 PDT

Created attachment 70557 [details] Patch

Andy Estes

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

Adam Barth

Comment 25 2010-10-12 13:16:11 PDT

> ..., which works. Cool! Shall we go with this approach then?

Andy Estes

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

Adam Barth

Comment 27 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?

Andy Estes

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

Andy Estes

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

Eric Seidel (no email)

Comment 30 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. :)

Darin Adler

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

Andy Estes

Comment 32 2010-10-12 16:45:09 PDT

Thanks for the review Darin. I'll clean up the style issues before landing.

Andy Estes

Comment 33 2010-10-12 17:25:30 PDT

Committed r69622: <http://trac.webkit.org/changeset/69622>

Note You need to log in before you can comment on or make changes to this bug.