Bug 86077 - Viewport attribute values are leaked when document.write() is used.
Summary: Viewport attribute values are leaked when document.write() is used.
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Critical
Assignee: zalan
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 79668 85425
  Show dependency treegraph
 
Reported: 2012-05-10 02:36 PDT by Csaba Osztrogonác
Modified: 2014-02-03 03:50 PST (History)
6 users (show)

See Also:


Attachments
test case (346 bytes, text/html)
2012-05-10 08:07 PDT, zalan
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 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]
Comment 1 zalan 2012-05-10 02:55:32 PDT
looking at it now
Comment 2 zalan 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.
Comment 3 zalan 2012-05-10 08:07:54 PDT
Created attachment 141173 [details]
test case
Comment 4 Csaba Osztrogonác 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.
Comment 5 zalan 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?
Comment 6 Kenneth Rohde Christiansen 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.
Comment 7 Antonio Gomes 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?
Comment 8 zalan 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())
Comment 9 zalan 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)
Comment 10 Jocelyn Turcotte 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.