Last Comment Bug 313278 - <script xmlns="http://www.w3.org/1999/xhtml"> leaks namespace to younger siblings
: <script xmlns="http://www.w3.org/1999/xhtml"> leaks namespace to younger sibl...
Status: VERIFIED FIXED
: fixed1.8
Product: Core
Classification: Components
Component: XML (show other bugs)
: Trunk
: x86 All
: -- major (vote)
: ---
Assigned To: Peter Van der Beken [:peterv]
: Ashish Bhatt
Mentors:
Depends on:
Blocks: 192139
  Show dependency treegraph
 
Reported: 2005-10-21 08:48 PDT by Benjamin Smedberg [:bsmedberg]
Modified: 2005-10-22 04:53 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (238 bytes, text/xml)
2005-10-21 08:51 PDT, Benjamin Smedberg [:bsmedberg]
no flags Details
v1 (3.41 KB, patch)
2005-10-21 09:39 PDT, Peter Van der Beken [:peterv]
bzbarsky: review+
bzbarsky: superreview+
asa: approval1.8rc1+
Details | Diff | Splinter Review
Nonempty tag testcase (256 bytes, application/xml)
2005-10-21 10:40 PDT, Boris Zbarsky [:bz]
no flags Details

Description Benjamin Smedberg [:bsmedberg] 2005-10-21 08:48:20 PDT
See attached testcase, basically

<element xmlns="1">
  <script xmlns="xhtml" src="foo" />
  <foo />
</element>

Foo has the wrong namespace. Which ends up screwing some significant number of
published SVG examples and a lot of mixed XHTML+SVG content.
Comment 1 Benjamin Smedberg [:bsmedberg] 2005-10-21 08:51:11 PDT
Created attachment 200350 [details]
Testcase
Comment 2 Boris Zbarsky [:bz] 2005-10-21 08:51:55 PDT
This regressed between 2004-12-15-06 and 2004-12-16-08, so looks like we somehow
fail to pop off the default namespace after the expat landing...
Comment 3 Boris Zbarsky [:bz] 2005-10-21 09:01:02 PDT
At a guess, we're adding the namespace binding, then bailing out when we block,
and never removing the binding...

Perhaps this is xmlparse.c line 2324?  We call the endElementHandler in line
2321, but we want to make it down to line 2341 before returning the error, no? 
Or do we not want to clear the tempPool here?  We definitely want that loop over
the bindings, I'd think.
Comment 4 Peter Van der Beken [:peterv] 2005-10-21 09:39:41 PDT
Created attachment 200359 [details] [diff] [review]
v1

It's going to be hard for me to test this soon, so if anyone could verify that
this fixes the problem I'd be grateful.
Comment 5 Boris Zbarsky [:bz] 2005-10-21 10:40:33 PDT
Created attachment 200363 [details]
Nonempty tag testcase
Comment 6 Boris Zbarsky [:bz] 2005-10-21 10:41:23 PDT
Comment on attachment 200359 [details] [diff] [review]
v1

Yeah, that fixes both testcases. r+sr=bzbarsky.

Can you check this in, or do you want me to?
Comment 7 Asa Dotzler [:asa] 2005-10-21 11:46:09 PDT
please land and verify on the trunk. 
Comment 8 Peter Van der Beken [:peterv] 2005-10-21 11:47:42 PDT
If you could, I'm off for the night.
Comment 9 Boris Zbarsky [:bz] 2005-10-21 12:08:35 PDT
Fixed on trunk.
Comment 10 Boris Zbarsky [:bz] 2005-10-21 12:10:05 PDT
Comment on attachment 200359 [details] [diff] [review]
v1

Requesting 1.8 branch approval.  This is a pretty serious regression in our XML
parsing.  The fix is pretty safe -- just move our early returns to be not quite
so early to allow expat to pop the namespaces associated with the just-closed
element.
Comment 11 Benjamin Smedberg [:bsmedberg] 2005-10-21 12:50:20 PDT
Verified fixed on trunk, we should definitely take this on branch.
Comment 12 Peter Van der Beken [:peterv] 2005-10-22 04:53:04 PDT
Checked in on branch. Thanks bz for your help.

Note You need to log in before you can comment on or make changes to this bug.