Bug 60685 – Solar Walk For Mac: Info window is blank (HTML5 parser)

• Component: New Bugs
• Status: RESOLVED FIXED

[Adele Peterson]

Created attachment 93238 [details]
patch

<rdar://problem/9253454>

The HTML content in the info window uses a self closing title tag.

Attaching a patch.  I don't normally touch the parser, so I'm not entirely sure of this.  I did test with the app and saw that it fixes the problem.  I also added a test and saw this passes existing tests.

[WebKit Review Bot]

Attachment 93238 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/mac/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Would this be easier to do via one of the JS-based quirks where we auto-inject js into the page?  I'm not sure that could fix this bug though, since I am not sure what would happen if we let everything get parsed inside the <title> and then move it out.

[Adele Peterson]

Good question - I'm not that familiar with that technique.  I'll look into it.

[Adam Barth]

Comment on attachment 93238 [details]
patch

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

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2734
> +        if (m_usePreHTML5ParserQuirks && token.selfClosing()) {

I don't think this what the old parser used to do. The old parser used to "re-parse" the document if the <title> tag ate the whole document.  I wonder if there's a more general solution that would apply to more apps than just solar walk.  In some sense, this is a "double mistake" by the author.  If we can treat the primary mistake (unterminated title tags), that's a mistake that's more likely to have been hit by other apps.

[Adam Barth]

That could lead to a more general solution.  Basically, you'll want to grab the text content of the title tag and document.write it at the end of the document.  That will be a much more faithful simulation of re-parsing than what you've got in this patch.

[Eric Seidel (no email)]

We have a js-based quirks system using user-script injection.  Sam or Andy should know more.

[Mark Rowe (bdash)]

This application is the only case where the difference in <title> parsing behavior has caused a problem since the HTML5 parser was enabled.  If this quirk were to be more widely needed than this single application then it could be beneficial to consider a more general solution.  Since it isn't I think the simple, targeted fix is reasonable.

[Adam Barth]

Comment on attachment 93238 [details]
patch

Marking r- while Adele investigates using the content script approach.  Please feel free to renominate for review if you decide that this approach is better.

[Eric Seidel (no email)]

See:
http://trac.webkit.org/browser/trunk/Source/WebKit/mac/Misc/MailQuirksUserScript.js
http://trac.webkit.org/browser/trunk/Source/WebKit/mac/Misc/OutlookQuirksUserScript.js

For examples of how to do quirks with user-scripts.  I suspect we could do something similar here, with an UnclosedTitleQuirkUserScript.js:
document.write(document.title)

Might just fix the bug with that one line js quirk. :)

[Eric Seidel (no email)]

(If you look at the changesets where those js quirks were added, you can see how WebKit goes about injecting them into Mail and Outlook respectively.)

[Adele Peterson]

When you use these user scripts, is there any way to test the quirk in DRT?

[Eric Seidel (no email)]

Not really.  Part of the point of the user script thing is that it makes the fix platform-only.  One doesn't have to change WebCore at all. :)

[Adele Peterson]

Created attachment 93377 [details]
patch

This patch uses the User Script approach.  I've manually tested Solar Walk with this change.

[Adam Barth]

Comment on attachment 93377 [details]
patch

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

> Source/WebKit/mac/Misc/SolarWalkQuirksUserScript.js:34
> + window.onload = reparseTitle;

I'd use window.addEventListender("load", reparseTitle, false) to make sure we don't get in the way of anyone else who's touching window.onload.

> Source/WebKit/mac/WebView/WebView.mm:678
> +    NSString *scriptPath = [[NSBundle bundleForClass:[WebView class]] pathForResource:@"SolarWalkQuirksUserScript" ofType:@"js"];

Would it make sense to call this reparseTitle.js in case apps besides solar walk need this quirk?  I guess we can rename it in the future were that to happen.

[Adele Peterson]

Created attachment 93384 [details]
patch

Fixes up the JS.  I didn't rename the file because so far all of our user scripts are per-app, and we might need to add further quirks for this application.   Not sure which will come first - more quirks for this app, or more apps that need this quirk.  We'll see.

[Darin Adler]

Comment on attachment 93384 [details]
patch

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

> Source/WebKit/mac/WebView/WebView.mm:686
> +    NSLog(@"contents of user script: %@\n", solarWalkQuirksScriptContents);

Probably should take out this log before landing.

> Source/WebKit/mac/WebView/WebView.mm:688
> +    core(self)->group().addUserScriptToWorld(core([WebScriptWorld world]),
> +                                             solarWalkQuirksScriptContents, KURL(), nullptr, nullptr, InjectAtDocumentEnd, InjectInAllFrames);

WebKit style says not to line subsequent lines up like this. (Too bad editors all seem to want to do it.)

[Adele Peterson]

Committed http://trac.webkit.org/changeset/86407

[Eric Seidel (no email)]

Fantastic!

FYI, I believe these scripts are injected right before onload? (I could be wrong, Sam could correct me.) in which case I doubt the onload hook was needed (aka, we might just have been able to run the scirpt directly).

Thanks for doing this Adele!

[Adele Peterson]

I hit a loader assertion when trying to document.write immediately.  So the event handler fixed that up.

(In reply to comment #18)
> Fantastic!
> 
> FYI, I believe these scripts are injected right before onload? (I could be wrong, Sam could correct me.) in which case I doubt the onload hook was needed (aka, we might just have been able to run the scirpt directly).
> 
> Thanks for doing this Adele!

[Simon Fraser (smfr)]

This caused some build warnings:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/29796/steps/compile-webkit/logs/warnings%20%282%29

[Adele Peterson]

Dan fixed the warning for me:
http://trac.webkit.org/changeset/86639

[Andy Estes]

I found a bug with this script where it is accidentally removing stylesheets from the document (since document.write() overwrites the <style> nodes that came before <title />).

I posted a fix at <https://bugs.webkit.org/show_bug.cgi?id=64777>.