387466 - Bug 309020 has regressed

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

The testcase in bug 309020 doesn't work (after being saved and loaded locally).  Neither does the testcase at http://dev.23inch.de/svg/marker/example.xml, for similar reasons.

The basic problem is that the URI passed to nsContentUtils::GetReferencedElement is the URI of the bound document, with the ref tossed on.  We discover that there's XBL involved and set the document URL to the URI of the binding document.  Then the URIs don't match, so we bail out.

I'm assuming this code did work at some point, but I'm not sure what change to make it not work -- the builds in my nightly archive that go back that far are built with --disable-svg.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that both the testcases in bug 309020 and the ones in the m.d.t.svg thread with the subject "markers & XBL" would make excellent reftests.

Comment 2

[Robert Longson [:longsonr]]

(In reply to comment #0)

To restate your comments in concrete terms

In http://dev.23inch.de/svg/marker/example.xml

The URI is http://dev.23inch.de/svg/marker/example.xml#Triangle

The document URL is calculated to be:
http://dev.23inch.de/svg/marker/example.xml

But for XBL we set it to the URI of the binding document which we determine is:
http://dev.23inch.de/svg/marker/example.xbl

So...

Is the URI incorrect? It comes like that out of the style system c.f. nsSVGPathGeometryFrame::UpdateMarkerProperty

Note that if we don't change the binding document i.e. remove the documentURL = do_QueryInterface(binding->PrototypeBinding()->DocURI()); and just set the isXBL flag then the marker is displayed.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Is the URI incorrect?

I have no idea.  Depends on how this stuff is designed to work.  That's why I cced the folks who wrote/reviewed it.

> It comes like that out of the style system

Which just uses whatever base URI the caller passes to it when resolving URIs during parsing.  With XBL1, this will generally be the URI of the bound document unless someone jumps through hoops.

Comment 4

[Robert Longson [:longsonr]]

Attachment: possible patch

This patch also fixes bug 399117. I don't know why this change is necessary though.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm foncused. So in this test:

  if (!EqualExceptRef(url, documentURL)) {

What are the values of url and documentURL, currently?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Currently:

* url is the URI of the ownerDocument of the node that the binding is attached to
* documentURL is the URI of the binding document

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So in this case, both values should be http://dev.23inch.de/svg/marker/example.xbl, I think. I don't understand why they're not equal.

Comment 8

[Boris Zbarsky [:bzbarsky]]

When GetReferencedElement is called, I see:

(gdb) p aURI->mSpec.mData
$5 = 0x8912ae8 "http://dev.23inch.de/svg/marker/example.xml#Triangle"
(gdb) p isXBL
$6 = 1
(gdb) p documentURL.mRawPtr->mSpec.mData
$7 = 0x8a8b4c0 "http://dev.23inch.de/svg/marker/example.xbl"

aURI is  GetStyleSVG()->mMarkerEnd.  It looks like the style comes from a "marker-end" atribute.

So presumably the question is when that attribute is actually turned into a content style rule .  And I further presume that this happens after the cloning that we use to install XBL anon content, or that said cloning doesn't preserve the style rule or something.  So when we parse it, we end up using the bound document as the source of the base URI.

Comment 9

[Boris Zbarsky [:bzbarsky]]

nsSVGElemen::UpdateContentStyleRule is what does the parsing.  It's been lazy all along as far as I can tell...  At least since 2004-03-09.  So I'm still not sure how this used to work.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Perhaps for XBL anon content UpdateContentStyleRule should use the binding document for the base URI?  That will fail with xml:base, but that seems like a rarer case...

Alternately, we could demand that binding content set xml:base to the binding document if they want to refer to it.

Comment 11

[Robert Longson [:longsonr]]

Attachment: possible comment 10 patch

An attempt to implement bz's first suggestion in comment 10.

Comment 12

[Robert Longson [:longsonr]]

Attachment: possible comment 10 patch

This time with all the changed files.

Comment 13

[Reed Loden [:reed]]

Comment on attachment 289939 [details] [diff] [review]
possible comment 10 patch

This bug is already blocking1.9+. No need for approval.

Comment 14

[Robert Longson [:longsonr]]

> This bug is already blocking1.9+. No need for approval.

So it is. Oops.

Checked in.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Can we please check in some tests so this won't regress again?

Comment 16

[Robert Longson [:longsonr]]

Attachment: reftest

Will this do you? I couldn't get the xul to match a single SVG file.

Comment 17

[Robert Longson [:longsonr]]

All I did was create two versions of the xul testcase, one with a marker and one without and check that they are different. If they are then presumably that is because the marker is affecting the rendering.

Comment 18

[Boris Zbarsky [:bzbarsky]]

It should be pretty easy to write one testcase with XBL and one without, no?  Just take any node that has a binding attached, replace that with an element that has the namespace/localname that's in extends= there, then replace its kids with the <content> of the binding, etc.  That is, do by hand the flattened tree that XBL ends up constructing in the testcase.

I think that's what the test for this should probably look like.  That's what http://dev.23inch.de/svg/marker/example.xml and http://dev.23inch.de/svg/marker/example2.xml are, no?  I thought I cited that testcase when filing the bug...

Better yet would be to avoid using extends="" altogether, since we plan to remove support for it.  Better to just have some SVG, some of which has XBL bound to it and markers inserted in the anonymous content.  Then have the same SVG with non-anonymous markers and the two should look the same.

Comment 19

[Jonathan Watt [:jwatt]]

Is there such a thing as "static" reflow? *g*

Just a thought. One of the reasons I created a separate 'svg' directory for SVG tests was so we could have a collection of tests to share with other SVG implementers (to improve interoperability). With that in mind it's probably useful to create a sub-subdirectory (or directories) for SVG that contains things other implementers may not be interested in (e.g. XUL or XBL). They'll be more likely to look at our tests if they don't have to wade through failing tests that only fail because they contain some XUL or whatever.

Perhaps we should create a subdirectory in 'svg' called 'moz-only' for this sort of thing? We can tidy its content into better sections later (or even just create it later - don't feel you have to do it now) but it would be good to start separating "other" tests.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Is there such a thing as "static" reflow? *g*

Not sure what you're asking here...

Comment 21

[Jonathan Watt [:jwatt]]

Sorry, that was a little joke about the file name "dynamic-reflow-01.svg". Ignore me on that one.

Comment 22

[Boris Zbarsky [:bzbarsky]]

For what it's worth, there is initial reflow and incremental reflow...  ;)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 290232 [details] [diff] [review]
reftest

Per comments

Comment 24

[Robert Longson [:longsonr]]

(In reply to comment #18)
> It should be pretty easy to write one testcase with XBL and one without, no? 
> Just take any node that has a binding attached, replace that with an element
> that has the namespace/localname that's in extends= there, then replace its
> kids with the <content> of the binding, etc.  That is, do by hand the flattened
> tree that XBL ends up constructing in the testcase.
> 
> I think that's what the test for this should probably look like.  That's what
> http://dev.23inch.de/svg/marker/example.xml and
> http://dev.23inch.de/svg/marker/example2.xml are, no?  I thought I cited that
> testcase when filing the bug...
> 

No matter what I tried, I just couldn't make plain SVG and XBL generated SVG reftests match each other. They look the same to me, but they won't match.

Anyway jwatt added some reftests which cover binding. These fail without the patch in this bug and pass with it so I'm hoping that's enough to mark this bug as in-testsuite+

reftests/svg/moz-only/xbl-basic-01.svg
reftests/svg/moz-only/xbl-basic-02.svg
reftests/svg/moz-only/xbl-grad-ref--grad-in-binding-01.svg
reftests/svg/moz-only/xbl-grad-ref--grad-in-binding-02.svg
reftests/svg/moz-only/xbl-grad-ref--grad-in-bound-01.svg
reftests/svg/moz-only/xbl-grad-ref--grad-in-bound-02.svg
reftests/svg/moz-only/xbl-grad-ref--grad-in-resources-01.svg
reftests/svg/moz-only/xbl-grad-ref--grad-in-resources-02.svg

Comment 25

[Boris Zbarsky [:bzbarsky]]

Yeah, those cover this bug.  Thanks!

Comment 26

[Loune]

Has this bug been fixed for firefox 3.0 beta 2? I'm getting negative results on it with the test case in Bug 309020.

Comment 27

[Robert Longson [:longsonr]]

Although the final patch fixed http://dev.23inch.de/svg/marker/example.xml it did not fix the testcase in bug 309020.

The testcase in bug 309020 still results in a documentURL of file:///xxx/test.xml and a URL of file:///xxx/test.xhtml#linearGradient2228 

Although we replace the baseURI in UpdateContentStyleRule with file:///xxx/test.xml that has no effect as test.xml has no properties unlike example.xbl in which marker-end is a property of a marker element.

Changing test.xml to have fill="url(#linearGradient2228)" rather than style="fill:url(#linearGradient2228)" makes things partially work as fill is a property but linearGradient2222 is still broken and I don't see a workaround for that.

FWIW The original possible patch would have fixed both examples.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Perhaps we need to reopen this bug then, depending on what our desired behavior is...

Comment 29

[Boris Zbarsky [:bzbarsky]]

Fwiw, I'd assumed that the patch fixed both testcases the bug was filed for...  I guess I shouldn't assume things like that.  :(

Comment 30

[Robert Longson [:longsonr]]

It did to begin with ;-)

Comment 31

[Boris Zbarsky [:bzbarsky]]

Well, it sounds like inline style parsing needs a similar change, perhaps.  That's not even SVG-specific, so much; just a general problem with inline style with XBL.

Should I reopen this bug or file a new one on the still-unfixed problem?

Comment 32

[Robert Longson [:longsonr]]

I guess reopening is warranted since the patch checked in so far is only a 50% solution to the issue.

Comment 33

[Robert Longson [:longsonr]]

Attachment: fix for both examples

This fixes both testcases. 

overriding GetBaseURI makes things work for non-attribute styles and also xlink:href parsing which is in nsSVGGradientFrame and other places.

Also all the styles which depend on baseURI need updating when the document is bound.

Comment 34

[Robert Longson [:longsonr]]

The question I have is should I have made the BindToTree change in nsSVGElement rather than nsStyledElement. I guess there could be performance implications for non SVG in what I've done in this patch.

Comment 35

[Boris Zbarsky [:bzbarsky]]

That code change will affect insertion of HTML nodes with inline style... ideally the inline style would know which base URI was used to parse it and we'd only reparse if that's changed.  Until we do that, better to limit the effect to SVG, I think.

Comment 36

[Robert Longson [:longsonr]]

Attachment: SVG only change

Comment 37

[Robert Longson [:longsonr]]

Attachment: check oldVal is not null since it is dereferenced

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 294478 [details] [diff] [review]
check oldVal is not null since it is dereferenced

Boris is really the better reviewer for this

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 294478 [details] [diff] [review]
check oldVal is not null since it is dereferenced

>Index: nsSVGElement.cpp
>+  nsIDocument* doc = GetOwnerDoc();
>+  if (doc) {
>+    nsIContent* bindingParent = GetBindingParent();
>+    if (bindingParent) {
>+      nsXBLBinding* binding = doc->BindingManager()->GetBinding(bindingParent);

You really want to use the ownerDocument of the binding parent here, I would think.  With XBL2, that will be different from the ownerDocument of the node.  I know that's not what the existing code did, but it's the right thing to do.

>+  nsIURI *base = baseURI;
>+  NS_IF_ADDREF(base);
>+  return base;

How about:

  return baseURI.forget();

to not do the extra refcount?

Witht those changes, r+sr=bzbarsky

Comment 40

[Robert Longson [:longsonr]]

Attachment: address review comments

Fairly low risk. The bindToTree code is taken from the base case with slightly changed logic. The rest of the code is simply moved so it affects non style attributes.

Comment 41

[Reed Loden [:reed]]

Comment on attachment 294974 [details] [diff] [review]
address review comments

This is a blocker. No approval needed. :)

Comment 42

[Robert Longson [:longsonr]]

Comment on attachment 294974 [details] [diff] [review]
address review comments

checked in.