Bug 86077

Summary: Viewport attribute values are leaked when document.write() is used.
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: Tools / TestsAssignee: alan <zalan>
Status: RESOLVED INVALID    
Severity: Critical CC: hugo.lima, kenneth, kpiascik, ossy, tonikitoo, zalan
Priority: P1 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 79668, 85425    
Attachments:
Description Flags
test case none

Csaba Osztrogonác
Reported 2012-05-10 02:36:29 PDT
The new assertion is introduced in r116571 and hit from the beginning. crash log for DumpRenderTree (pid 21943): STDOUT: <empty> STDERR: ASSERTION FAILED: m_viewportArguments.type == ViewportArguments::Implicit STDERR: ../../../../Source/WebCore/dom/Document.cpp(777) : void WebCore::Document::setDocType(WTF::PassRefPtr<WebCore::DocumentType>) STDERR: 1 0x7fce283a07c8 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::Document::setDocType(WTF::PassRefPtr<WebCore::DocumentType>)+0x11c) [0x7fce283a07c8] STDERR: 2 0x7fce283eff6d /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::DocumentType::insertedInto(WebCore::Node*)+0x13f) [0x7fce283eff6d] STDERR: 3 0x7fce28382258 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::ChildNodeInsertionNotifier::notifyNodeInsertedIntoDocument(WebCore::Node*)+0x9a) [0x7fce28382258] STDERR: 4 0x7fce283823d7 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::ChildNodeInsertionNotifier::notifyInsertedIntoDocument(WebCore::Node*)+0x23) [0x7fce283823d7] STDERR: 5 0x7fce28384e3f /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::ContainerNode::parserAddChild(WTF::PassRefPtr<WebCore::Node>)+0x14d) [0x7fce28384e3f] STDERR: 6 0x7fce28665d82 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(+0x25aed82) [0x7fce28665d82] STDERR: 7 0x7fce28666144 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLConstructionSite::executeQueuedTasks()+0x76) [0x7fce28666144] STDERR: 8 0x7fce28666f99 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLConstructionSite::insertDoctype(WebCore::AtomicHTMLToken&)+0x2b5) [0x7fce28666f99] STDERR: 9 0x7fce2868eeaa /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLTreeBuilder::processDoctypeToken(WebCore::AtomicHTMLToken&)+0x86) [0x7fce2868eeaa] STDERR: 10 0x7fce2868ed89 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLTreeBuilder::processToken(WebCore::AtomicHTMLToken&)+0xa5) [0x7fce2868ed89] STDERR: 11 0x7fce2868ebe8 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken(WebCore::AtomicHTMLToken&)+0x50) [0x7fce2868ebe8] STDERR: 12 0x7fce2868eae2 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLTreeBuilder::constructTreeFromToken(WebCore::HTMLToken&)+0x66) [0x7fce2868eae2] STDERR: 13 0x7fce2866df3b /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)+0x327) [0x7fce2866df3b] STDERR: 14 0x7fce2866d801 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode)+0xb1) [0x7fce2866d801] STDERR: 15 0x7fce2866e370 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLDocumentParser::insert(WebCore::SegmentedString const&)+0xa4) [0x7fce2866e370] STDERR: 16 0x7fce283a8b5a /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::Document::write(WebCore::SegmentedString const&, WebCore::Document*)+0x1ac) [0x7fce283a8b5a] STDERR: 17 0x7fce2811143e /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(+0x205a43e) [0x7fce2811143e] STDERR: 18 0x7fce281114dd /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::JSHTMLDocument::write(JSC::ExecState*)+0x33) [0x7fce281114dd] STDERR: 19 0x7fce29281fc7 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::jsHTMLDocumentPrototypeFunctionWrite(JSC::ExecState*)+0x12b) [0x7fce29281fc7] STDERR: 20 0x7fcddc620265 [0x7fcddc620265]
Attachments
test case (346 bytes, text/html)
2012-05-10 08:07 PDT, alan 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
alan
Comment 1 2012-05-10 02:55:32 PDT
looking at it now
alan
Comment 2 2012-05-10 05:27:09 PDT
1, Document is reused multiple times by injecting new content using document.write(). 2, content A has xhtml-mobile dtd and document sets the legacy viewport attributes. 3, content B comes and assert hits because the viewport attribute type leaks from content A. it's good that the type check was replaced by the assert, as with the type check, we would have missed proper viewport attribute adaptation, while content A with 'meta viewport' -> content B with 'xhtml-mobile' transition.
alan
Comment 3 2012-05-10 08:07:54 PDT
Created attachment 141173 [details] test case
Csaba Osztrogonác
Comment 4 2012-05-11 00:15:47 PDT
I skipped the asserting test - https://trac.webkit.org/changeset/116734 Please unskip it with the proper fix.
alan
Comment 5 2012-05-14 06:07:36 PDT
this gets a little bit more generic: given the following meta <meta id="viewport" name='viewport' content='width=device-width'> these 2 calls both update viewport values to default: document.getElementById("viewport").setAttribute("content", ""); document.getElementsByTagName("meta")[0].content=''; however, none of the calls below initiate viewport value update: a, var vp = document.getElementById("viewport"); vp.parentNode.removeChild(vp); b, document.getElementsByTagName("head")[0].innerHTML = ""; or even when the entire content of the document is trashed using document.open() -> document.write() Shouldn't dynamically removed meta viewport initiate update on the viewport values similarly when the 'content' attribute is changed?
Kenneth Rohde Christiansen
Comment 6 2012-05-14 07:06:36 PDT
(In reply to comment #5) > this gets a little bit more generic: > > given the following meta > <meta id="viewport" name='viewport' content='width=device-width'> > > these 2 calls both update viewport values to default: > document.getElementById("viewport").setAttribute("content", ""); > document.getElementsByTagName("meta")[0].content=''; > > however, none of the calls below initiate viewport value update: > a, var vp = document.getElementById("viewport"); vp.parentNode.removeChild(vp); > b, document.getElementsByTagName("head")[0].innerHTML = ""; > > or even when the entire content of the document is trashed using > document.open() -> document.write() > > Shouldn't dynamically removed meta viewport initiate update on the viewport values similarly when the 'content' attribute is changed? This sounds sensible to me.
Antonio Gomes
Comment 7 2012-05-18 21:19:44 PDT
(In reply to comment #6) > (In reply to comment #5) > > this gets a little bit more generic: > > > > given the following meta > > <meta id="viewport" name='viewport' content='width=device-width'> > > > > these 2 calls both update viewport values to default: > > document.getElementById("viewport").setAttribute("content", ""); > > document.getElementsByTagName("meta")[0].content=''; > > > > however, none of the calls below initiate viewport value update: > > a, var vp = document.getElementById("viewport"); vp.parentNode.removeChild(vp); > > b, document.getElementsByTagName("head")[0].innerHTML = ""; > > > > or even when the entire content of the document is trashed using > > document.open() -> document.write() > > > > Shouldn't dynamically removed meta viewport initiate update on the viewport values similarly when the 'content' attribute is changed? > > This sounds sensible to me. +1. how does firefox mobile or opera behavior with dynamic viewport values?
alan
Comment 8 2012-05-22 07:40:05 PDT
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > this gets a little bit more generic: > > > > > > given the following meta > > > <meta id="viewport" name='viewport' content='width=device-width'> > > > > > > these 2 calls both update viewport values to default: > > > document.getElementById("viewport").setAttribute("content", ""); > > > document.getElementsByTagName("meta")[0].content=''; > > > > > > however, none of the calls below initiate viewport value update: > > > a, var vp = document.getElementById("viewport"); vp.parentNode.removeChild(vp); > > > b, document.getElementsByTagName("head")[0].innerHTML = ""; > > > > > > or even when the entire content of the document is trashed using > > > document.open() -> document.write() > > > > > > Shouldn't dynamically removed meta viewport initiate update on the viewport values similarly when the 'content' attribute is changed? > > > > This sounds sensible to me. > > +1. how does firefox mobile or opera behavior with dynamic viewport values? FF mobile(14.0?) on Android(4.0.4) does not seem to be supporting any dynamic change of the viewport values (and it is broken in so many ways) However, Opera mobile(12.0) on Android(4.0.4) seems to be supporting changing the viewport values with 1, setAttribute, 2, removing the viewport using removeChild() -though it gets confused with multiple viewport metas. 3, emptying the <head> using document.getElementsByTagName("head")[0].innerHTML = ""; 4, and even using document.write() -though the new content's viewport values are ignored, so it looks like it just resets the viewport to default (as if no meta present) So Opera mobile clearly reacts on when the meta tag changes unlike us (other than the setAttribute())
alan
Comment 9 2012-05-22 07:49:48 PDT
We need to be clever about this though as multiple viewport meta tags in the document can make things complicated, such as 1, changing the ignored ones should not have any impact on the viewport. 2, after removing the one was in use, some priority order need to be checked to see which one to take next. 3, adding a new one might or might not change the actual viewport values. (later when we introduce legacy tags to get converted into viewport values)
Jocelyn Turcotte
Comment 10 2014-02-03 03:50:53 PST
=== Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.
Note You need to log in before you can comment on or make changes to this bug.