Bug 43836 – Delete LegacyHTMLDocumentParser and LegacyPreloadScanner

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Delete LegacyHTMLDocumentParser and LegacyPreloadScanner

[Adam Barth]

Created attachment 64086 [details]
Patch

[Adam Barth]

Created attachment 64087 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 64087 [details]
Patch

WebCore/dom/DocumentParser.h:62
 +      // used to implements it.
used to implement

My name is Eric Seidel, and I support this message.

[Darin Adler]

At Apple we’re starting to get multiple bug reports about Mac OS X applications and websites that don’t work any more due to differences between the old and new parser.

One example is AOL’s AIM client, which malfunctions in a spectacular way. But I’m sure more of these reports are coming as people have more time to test.

Once you delete the old parser, we will presumably have to add hacks to the new one to make that old content work. Is that your preferred approach?

[Adam Barth]

> One example is AOL’s AIM client, which malfunctions in a spectacular way. But I’m sure more of these reports are coming as people have more time to test.

Yes.  Some of these reports have filtered into bugs.webkit.org as bugs blocking Bug 41115.  If you're able to share the reports (or sanitized versions of the reports), filing them as blocking Bug 41115 will make sure they don't get lost.

> Once you delete the old parser, we will presumably have to add hacks to the new one to make that old content work. Is that your preferred approach?

We talked this over with Alexey at some point.  We think that's a better approach because maintaining branches in the new parser seems easier then maintaining an entirely separate parser.

As an example, Chrome decided to ship the old parser in the upcoming M6 beta because we're not quite done with the new parser yet.  When they turned it on, they found that it had already bit rotted to the point of a use-after-free on every document parse due to this change:

http://trac.webkit.org/changeset/62302/trunk/WebCore/html/LegacyHTMLDocumentParser.cpp

Notice the comment at the top of the diff that says "this actually causes us to be deleted."  I don't mean to blame anyone by pointing out this patch.  It just illustrates the general principle that "if it's not tested, it's wrong."

[Adam Barth]

Comment on attachment 64087 [details]
Patch

Clearing flags on attachment: 64087

Committed r65167: <http://trac.webkit.org/changeset/65167>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/65167 might have broken Chromium Win Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/65166
http://trac.webkit.org/changeset/65167