Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
729453003:
DevTools: Fix crash when setting invalid outer XML for an XML document (Closed)
|
Created:
6 years, 1 month ago by apavlov Modified:
6 years, 1 month ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionDevTools: Fix crash when setting invalid outer XML for an XML document
R=pfeldman, vsevik
BUG=431519
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185454
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix the way documents are parsed, address comments #
Total comments: 2
Patch Set 3 : Revert to ASSERT, detach parser() after finish() #
Total comments: 5
Messages
Total messages: 23 (8 generated)
lgtm
The CQ bit was checked by apavlov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/729453003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)
aandrey@chromium.org changed reviewers: + aandrey@chromium.org
https://codereview.chromium.org/729453003/diff/1/LayoutTests/inspector/elemen... File LayoutTests/inspector/elements/set-outer-html-for-xhtml.xhtml (right): https://codereview.chromium.org/729453003/diff/1/LayoutTests/inspector/elemen... LayoutTests/inspector/elements/set-outer-html-for-xhtml.xhtml:56: InspectorTest.addResult("No crash"); nit: "PASS: No crash" https://codereview.chromium.org/729453003/diff/1/Source/core/inspector/DOMPat... File Source/core/inspector/DOMPatchSupport.cpp (right): https://codereview.chromium.org/729453003/diff/1/Source/core/inspector/DOMPat... Source/core/inspector/DOMPatchSupport.cpp:93: ASSERT(newDocument); maybe make it a RELEASE_ASSERT or put after: if (!newDocument) return; ?
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/729453003/diff/1/LayoutTests/inspector/elemen... File LayoutTests/inspector/elements/set-outer-html-for-xhtml.xhtml (right): https://codereview.chromium.org/729453003/diff/1/LayoutTests/inspector/elemen... LayoutTests/inspector/elements/set-outer-html-for-xhtml.xhtml:56: InspectorTest.addResult("No crash"); On 2014/11/14 16:30:53, aandrey wrote: > nit: "PASS: No crash" Done. https://codereview.chromium.org/729453003/diff/1/Source/core/inspector/DOMPat... File Source/core/inspector/DOMPatchSupport.cpp (right): https://codereview.chromium.org/729453003/diff/1/Source/core/inspector/DOMPat... Source/core/inspector/DOMPatchSupport.cpp:93: ASSERT(newDocument); On 2014/11/14 16:30:53, aandrey wrote: > maybe make it a RELEASE_ASSERT or put after: > > if (!newDocument) > return; > > ? Done.
The CQ bit was checked by apavlov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/729453003/40001
https://codereview.chromium.org/729453003/diff/40001/Source/core/inspector/DO... File Source/core/inspector/DOMPatchSupport.cpp (right): https://codereview.chromium.org/729453003/diff/40001/Source/core/inspector/DO... Source/core/inspector/DOMPatchSupport.cpp:95: RELEASE_ASSERT(newDocument); ASSERT?
The CQ bit was unchecked by apavlov@chromium.org
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/729453003/diff/40001/Source/core/inspector/DO... File Source/core/inspector/DOMPatchSupport.cpp (right): https://codereview.chromium.org/729453003/diff/40001/Source/core/inspector/DO... Source/core/inspector/DOMPatchSupport.cpp:95: RELEASE_ASSERT(newDocument); On 2014/11/17 14:10:04, pfeldman wrote: > ASSERT? Done.
The CQ bit was checked by apavlov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/729453003/80001
https://codereview.chromium.org/729453003/diff/80001/Source/core/inspector/DO... File Source/core/inspector/DOMPatchSupport.cpp (right): https://codereview.chromium.org/729453003/diff/80001/Source/core/inspector/DO... Source/core/inspector/DOMPatchSupport.cpp:96: newDocument->setContextFeatures(document().contextFeatures()); if a new document type will be added, it will crash here. maybe crash only in debug, not release? https://codereview.chromium.org/729453003/diff/80001/Source/core/inspector/DO... Source/core/inspector/DOMPatchSupport.cpp:105: if (!static_cast<XMLDocumentParser*>(parser.get())->wellFormed()) you can remove this static_cast with proper type for parser
https://codereview.chromium.org/729453003/diff/80001/Source/core/inspector/DO... File Source/core/inspector/DOMPatchSupport.cpp (right): https://codereview.chromium.org/729453003/diff/80001/Source/core/inspector/DO... Source/core/inspector/DOMPatchSupport.cpp:96: newDocument->setContextFeatures(document().contextFeatures()); On 2014/11/17 15:47:53, aandrey wrote: > if a new document type will be added, it will crash here. maybe crash only in > debug, not release? That's what ASSERT() is for. https://codereview.chromium.org/729453003/diff/80001/Source/core/inspector/DO... Source/core/inspector/DOMPatchSupport.cpp:105: if (!static_cast<XMLDocumentParser*>(parser.get())->wellFormed()) On 2014/11/17 15:47:53, aandrey wrote: > you can remove this static_cast with proper type for parser With a proper type for parser, I cannot call append() and finish(), since these API methods are private on XMLDocumentParser.
https://codereview.chromium.org/729453003/diff/80001/Source/core/inspector/DO... File Source/core/inspector/DOMPatchSupport.cpp (right): https://codereview.chromium.org/729453003/diff/80001/Source/core/inspector/DO... Source/core/inspector/DOMPatchSupport.cpp:96: newDocument->setContextFeatures(document().contextFeatures()); On 2014/11/17 15:47:53, aandrey wrote: > if a new document type will be added, it will crash here. maybe crash only in > debug, not release? That's what ASSERT() is for. https://codereview.chromium.org/729453003/diff/80001/Source/core/inspector/DO... Source/core/inspector/DOMPatchSupport.cpp:105: if (!static_cast<XMLDocumentParser*>(parser.get())->wellFormed()) On 2014/11/17 15:47:53, aandrey wrote: > you can remove this static_cast with proper type for parser With a proper type for parser, I cannot call append() and finish(), since these API methods are private on XMLDocumentParser.
https://codereview.chromium.org/729453003/diff/80001/Source/core/inspector/DO... File Source/core/inspector/DOMPatchSupport.cpp (right): https://codereview.chromium.org/729453003/diff/80001/Source/core/inspector/DO... Source/core/inspector/DOMPatchSupport.cpp:96: newDocument->setContextFeatures(document().contextFeatures()); On 2014/11/17 16:04:39, apavlov wrote: > On 2014/11/17 15:47:53, aandrey wrote: > > if a new document type will be added, it will crash here. maybe crash only in > > debug, not release? > > That's what ASSERT() is for. Eh, I see what you mean. I cannot think of a way to edit other kinds of documents :) Anyway, Pavel says ASSERT() is good enough.
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as 185454 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
