Bug 43836 - Delete LegacyHTMLDocumentParser and LegacyPreloadScanner
Summary: Delete LegacyHTMLDocumentParser and LegacyPreloadScanner
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:
Depends on:
Blocks:
 
Reported: 2010-08-11 01:16 PDT by Adam Barth
Modified: 2010-08-11 11:49 PDT (History)
6 users (show)

See Also:


Attachments
Patch (173.30 KB, patch)
2010-08-11 01:20 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (177.28 KB, patch)
2010-08-11 01:40 PDT, Adam Barth
no flags 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-08-11 01:16:37 PDT
Delete LegacyHTMLDocumentParser and LegacyPreloadScanner
Comment 1 Adam Barth 2010-08-11 01:20:18 PDT
Created attachment 64086 [details]
Patch
Comment 2 Adam Barth 2010-08-11 01:40:26 PDT
Created attachment 64087 [details]
Patch
Comment 3 Eric Seidel (no email) 2010-08-11 09:27:44 PDT
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.
Comment 4 Darin Adler 2010-08-11 09:30:30 PDT
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?
Comment 5 Adam Barth 2010-08-11 09:50:26 PDT
> 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."
Comment 6 Adam Barth 2010-08-11 11:31:22 PDT
Comment on attachment 64087 [details]
Patch

Clearing flags on attachment: 64087

Committed r65167: <http://trac.webkit.org/changeset/65167>
Comment 7 Adam Barth 2010-08-11 11:31:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 WebKit Review Bot 2010-08-11 11:49:50 PDT
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