Bug 39828 - document.write does not work correctly in the HTML5 parser
Summary: document.write does not work correctly in the HTML5 parser
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: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 39259
  Show dependency treegraph
 
Reported: 2010-05-27 04:37 PDT by Eric Seidel (no email)
Modified: 2010-05-28 03:11 PDT (History)
4 users (show)

See Also:


Attachments
work in progress (16.88 KB, patch)
2010-05-27 04:40 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (25.16 KB, patch)
2010-05-28 01:25 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Updated to TOT and removed tabs (25.18 KB, patch)
2010-05-28 01:52 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch for landing (24.67 KB, patch)
2010-05-28 02:54 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 Eric Seidel (no email) 2010-05-27 04:37:35 PDT
document.write does not work correctly in the HTML5 parser
Comment 1 Eric Seidel (no email) 2010-05-27 04:40:37 PDT
Created attachment 57222 [details]
work in progress
Comment 2 Eric Seidel (no email) 2010-05-27 04:42:36 PDT
Comment on attachment 57222 [details]
work in progress

Sorry, forgot the --no-review flag.

This patch has some extra junk in it and is not ready for review.

It passes the simple document.write cases, but hits the ASSERT(scriptElement) in bool HTML5ScriptRunner::execute(PassRefPtr<Element> scriptElement) on more complicated ones.  I think that's expected, I just need to sort through the exact conditions under which that ASSERT is bogus.
Comment 3 Eric Seidel (no email) 2010-05-28 01:25:43 PDT
Created attachment 57303 [details]
Patch
Comment 4 Eric Seidel (no email) 2010-05-28 01:26:47 PDT
This implementation is WAY simpler than the current document.write implementation in the old parser.  Either I did something wrong, or this is validation of our parser design. :)  I'm hopeful for the latter.
Comment 5 Eric Seidel (no email) 2010-05-28 01:33:43 PDT
This appears to have fixed our major crash-on-navigation issue which I believe was caused by failing to unregister as a client from a CachedResource and then having it notify a dead pointer. :)
Comment 6 Eric Seidel (no email) 2010-05-28 01:40:22 PDT
Actually, looking at this more, I think this will be an incomplete solution.  There are other codepaths through which we could re-enter scripting.  The various event dispatches being some.

We may need to move the event dispatch code onto the HTML5ScriptRunnerHost.
Comment 7 Eric Seidel (no email) 2010-05-28 01:42:14 PDT
fast/dom/document-clear.html still hits an ASSERT after this change, but many many more layout tests pass. :)
Comment 8 Eric Seidel (no email) 2010-05-28 01:52:09 PDT
Created attachment 57305 [details]
Updated to TOT and removed tabs
Comment 9 Adam Barth 2010-05-28 02:30:24 PDT
Comment on attachment 57305 [details]
Updated to TOT and removed tabs

Wow, that's very pretty.  :)  Some comments below.

WebCore/ChangeLog:12
 +          before calling ScriptControlle::executeScript to allow safe
ScriptControlle -> ScriptController

WebCore/html/HTML5Tokenizer.cpp:42
 +      , m_docuemnt(document)
m_docuemnt -> m_document

WebCore/html/HTML5Tokenizer.cpp:120
 +      // not an already-loaded CachedResource.
There's some grammar problems here.

WebCore/html/HTML5Tokenizer.cpp:131
 +      // Not currently possible to execute scripts on frameless documents.
It's not so much that it's not possible.  We explicitly don't want to.

WebCore/html/HTML5Tokenizer.cpp:135
 +      SegmentedString oldInsertionPoint = m_source;
How expensive is this copy?  Maybe we should use OwnPtr and swap?
Comment 10 Adam Barth 2010-05-28 02:54:41 PDT
Created attachment 57308 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2010-05-28 03:11:25 PDT
Comment on attachment 57308 [details]
Patch for landing

Clearing flags on attachment: 57308

Committed r60347: <http://trac.webkit.org/changeset/60347>
Comment 12 WebKit Commit Bot 2010-05-28 03:11:32 PDT
All reviewed patches have been landed.  Closing bug.