Bug 86077 – Viewport attribute values are leaked when document.write() is used.

• Status: RESOLVED
• Resolution: INVALID
• Component: Tools / Tests

[Csaba Osztrogonác]

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]

[zalan]

looking at it now

[zalan]

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.

[zalan]

Created attachment 141173 [details]
test case

[Csaba Osztrogonác]

I skipped the asserting test - https://trac.webkit.org/changeset/116734
Please unskip it with the proper fix.

[zalan]

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]

(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]

(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?

[zalan]

(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())

[zalan]

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]

=== 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.