Last Comment Bug 740811 - SVGSVGElement doesn't look into Element.prototype
: SVGSVGElement doesn't look into Element.prototype
Status: RESOLVED FIXED
[qa+]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Linux
: -- normal with 5 votes (vote)
: mozilla14
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
Depends on:
Blocks: 589640
  Show dependency treegraph
 
Reported: 2012-03-30 08:04 PDT by Arian Stolwijk
Modified: 2012-11-19 06:58 PST (History)
18 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
verified


Attachments
Element.svg.html (1.06 KB, text/html)
2012-03-30 08:04 PDT, Arian Stolwijk
no flags Details
Make SVGElement.prototype be Element.prototype again. (2.68 KB, patch)
2012-04-09 08:46 PDT, Boris Zbarsky [:bz] (still a bit busy)
peterv: review+
lukasblakk+bugs: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑central+
Details | Diff | Splinter Review

Description Arian Stolwijk 2012-03-30 08:04:19 PDT
Created attachment 610885 [details]
Element.svg.html

User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/535.19 (KHTML, like Gecko) Chrome/18.0.1025.142 Safari/535.19

Steps to reproduce:

I tried this code:

Element.prototype.test = 'test';
var svg = document.getElementsByTagName('svg')[0];
console.assert(svg.test == 'test');



Actual results:

The assertion failed


Expected results:

the assertion shouldn't have failed.
Comment 1 Arian Stolwijk 2012-03-30 08:06:05 PDT
This bug breaks MooTools Element methods on <svg> elements:
https://github.com/mootools/mootools-core/issues/2331
Comment 2 Daniel Buchner [:dbuc] 2012-03-30 08:22:29 PDT
Hey guys, any reason why SVG elements are failing to pick up their prototypes as Arian indicates? If this is not expected behavior, it could affect the assumptions and element-centric code of many developers, please advise.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-03-30 10:11:40 PDT
Looks like SVGElement.prototype has Object.prototype as its proto instead of Element.prototype.  I'll look into why on Monday if no one else has before then.

If this is a regression, someone finding the regression range would be a big help.
Comment 4 Arian Stolwijk 2012-03-31 13:59:06 PDT
Maybe interesting to note is that:

Element.prototype.test = 'yo'
document.createElementNS('http://www.w3.org/2000/svg', 'svg').test 
// → undefined in FF, but 'yo' in chrome
document.createElement('svg').test
// → 'yo' in both FF and Chrome
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2012-04-01 06:56:04 PDT
(In reply to Arian Stolwijk from comment #4)
> Maybe interesting to note is that:
> 
> Element.prototype.test = 'yo'
> document.createElementNS('http://www.w3.org/2000/svg', 'svg').test 
> // → undefined in FF, but 'yo' in chrome
> document.createElement('svg').test
> // → 'yo' in both FF and Chrome

The latter creates an HTMLElement.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-04-06 20:54:01 PDT
So the issue is that

   DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF(SVGUnknownElement, nsIDOMSVGElement)

means that when we're setting up the proto for SVGElement we end up using the interfaceinfo for nsIDOMSVGElement for the classname.  So we recursively look up "SVGElement" on the window, look for .prototype on it, get nothing (since we're still setting it up) and end up using Object.prototype as the proto.

Peter, is the NO_CLASS_IF part here correct?  Why do we not want to use the parent in the NO_CLASS_IF case, exactly?
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-04-06 20:55:35 PDT
Probably a regression from bug 82235.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-04-06 20:55:52 PDT
Probably a regression from bug 589640.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-04-07 20:58:15 PDT
The NO_CLASS_IF bit was added by me as a test bustage fix for bug 589640, but I didn't write down what was broken, sadly.

I tried pushing a backout of the NO_CLASS_IF bit to try, and it seems to pass tests...  It also fixes this bug as far as I can tell.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-04-09 08:46:24 PDT
Created attachment 613305 [details] [diff] [review]
Make SVGElement.prototype be Element.prototype again.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-04-19 12:41:33 PDT
Comment on attachment 613305 [details] [diff] [review]
Make SVGElement.prototype be Element.prototype again.

Requesting approval for m-c.  I think this should be a pretty safe fix, and it's a basic web compat issue that we should fix.
Comment 12 Peter Van der Beken [:peterv] 2012-04-19 13:39:46 PDT
Just for posterity, the reason removing NO_CLASS_IF is ok is that SVGUnknownElement is the DOMCI for SVGElement, so the primary interface is the class interface (nsIDOMSVGElement).
Comment 13 Alex Keybl [:akeybl] 2012-04-20 15:38:32 PDT
Comment on attachment 613305 [details] [diff] [review]
Make SVGElement.prototype be Element.prototype again.

[Triage Comment]
Approved for mozilla-central - this is a web compatibility bug, and we don't expect it to have a major effect on any of the main FN use cases.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2012-04-20 19:17:50 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba9898675c51
Comment 15 Phil Ringnalda (:philor) 2012-04-21 23:51:16 PDT
https://hg.mozilla.org/mozilla-central/rev/ba9898675c51
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-24 19:47:03 PDT
It seems to me that we should put this regression fix on aurora/beta as well.
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2012-04-24 20:00:56 PDT
Comment on attachment 613305 [details] [diff] [review]
Make SVGElement.prototype be Element.prototype again.

Yeah, that's not a bad idea.  I hadn't wanted to rush it into 12 before.

[Approval Request Comment]
Regression caused by (bug #): 589640 
User impact if declined: Some sites' JS doesn't work right.
Testing completed (on m-c, etc.): Checked in on m-c, passing tests.
Risk to taking this patch (and alternatives if risky): Slight risk that something
   could go wrong, but I'm not really sure what it would be.  Never bet against
   some website _somewhere_ breaking, though.  ;)
String changes made by this patch:
Comment 18 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-24 20:20:00 PDT
Comment on attachment 613305 [details] [diff] [review]
Make SVGElement.prototype be Element.prototype again.

This made the cutover.
Comment 19 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-25 12:02:58 PDT
[Triage Comment]
Approved for beta and updating the status flags to show it's not yet on beta (but made the cut to aurora) - please update flag once this has landed.
Comment 20 Alex Keybl [:akeybl] 2012-05-01 10:39:43 PDT
Can we move ahead with landing the approved patch on FF13?
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2012-05-01 10:43:03 PDT
Er, yes.  I didn't set the flag, so didn't get the notification mail....
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2012-05-01 10:46:59 PDT
Hm.  I _did_ set the flag.  I wonder what happened to that mail.

In any case, http://hg.mozilla.org/releases/mozilla-beta/rev/05cfe44826dd
Comment 23 Ioana (away) 2012-05-30 06:52:02 PDT
The automated test for this bug passed on all OSs on Firefox 13.0 beta:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12163160&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=12163930&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=12159488&full=1&branch=mozilla-beta

I have also tried to verify this fix manually and the assertion from comment 0 doesn't fail on Firefox 13.0b6. The problem is that it also doesn't fail on Firefox 12.0 on my workstation. 

Could someone please let me know how to reproduce this issue?
Until now I have tried loading the Element.svg.html attachment in Firefox 12.0 and looking in the Error and Web consoles for the assertion failure. Also tried with replacing the corresponding lines from that attachment with 

Element.prototype.test = 'test';
var svg = document.getElementsByTagName('svg')[0];
console.assert(svg.test == 'test');

and looking in the Error and Web consoles for the assertion failures.
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2012-05-30 07:28:19 PDT
> The problem is that it also doesn't fail on Firefox 12.0 on my workstation. 

That's because there is no console.assert method.

> Could someone please let me know how to reproduce this issue?

Replace console.assert by alert() and see what boolean gets alerted.
Comment 25 Ioana (away) 2012-05-30 07:52:01 PDT
Boris, thank you for you suggestion. I didn't get any error about using an inexistent method, so I didn't know for sure what the problem was.

Verified as fixed with the guidelines from comment 0 and comment 24:
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
BuildID: 20120528154913
Comment 26 Ioana (away) 2012-06-06 07:16:15 PDT
The automated test for this bug passed on all OSs on Firefox 14.0 beta too:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12407667&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=12407415&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=12410453&full=1&branch=mozilla-beta

Also verified as fixed with the guidelines from comments 0 and 24 on:
Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20100101 Firefox/14.0 (20120605113340)
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2012-11-19 06:58:13 PST
No point for developer docs for this.  It might have been useful in the Fx14 docs, but that ship has long sailed.

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