Closed
Bug 888225
Opened 11 years ago
Closed 11 years ago
firefox 22 breaks access to frames named 'sidebar'
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
People
(Reporter: philemon-bugzilla, Assigned: bholley)
References
Details
(Keywords: dev-doc-complete, regression, site-compat, Whiteboard: [qa-])
Attachments
(2 files, 4 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
bajaj
:
approval-mozilla-aurora+
bajaj
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release) Build ID: 20130620135945 Steps to reproduce: Have a website with an iframe named sidebar and try to access it via Javascript. I am going to attach files which demonstrate the problem. Actual results: The console displayed: TypeError: document.sidebar is undefined @ file:...demo.html:1 The browser content displays the text "test case: access to sidebar failed" which is the original content of my iframe (from "demo_sidebar.html"). Expected results: As with Firefox 21, no error should occur and the iframe should be updated. The browser content should display the text "test case: access to sidebar works".
Reporter | ||
Comment 1•11 years ago
|
||
Completing the test case
Reporter | ||
Comment 2•11 years ago
|
||
Sorry about the spam, but I didn't find a way to attach several files at once. The test case are 3 files: demo.hmtl - the main file to load in your browser demo_sidebar.html - the content given as iframe src demo_sidebar_2.html - the content set via javascript With Firefox 21 you see the content of demo_sidebar_2.html ("test case: access to sidebar works"), with Firefox 22 you get the unmodifed content from demo_sidebar.html ("test case: access to sidebar failed"). I guess that the change of behaviour has to do with the social networks changes to Firefox 22, but wouldn't expect that to break existing names?
Attachment #768876 -
Attachment mime type: text/plain → text/html
I tried with FF21 nightlies, same error. Could you test with a clean profil, please. https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles
Reporter | ||
Comment 4•11 years ago
|
||
(In reply to Loic from comment #3) [...] > I tried with FF21 nightlies, same error. Argl. Simplified the test case too much and didn't notice (not sure but I guess mixed it up with Chrome, which I also tested with). I'll try (hopefully) on Monday to create a new test case which properly shows the problem I am (still) observing here. Sorry for the inconvenience. > Could you test with a clean profil, please. > https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove- > firefox-profiles I can confirm that I still observe the problems when I completely remove the .mozilla directory and start anew when using Firefox 22, but not with Firefox 21. As I said I will try to create a working test case soon.
Reporter | ||
Comment 5•11 years ago
|
||
Thanks Loic for your rewrite to use only one file. But regardless of my mistake, I think you reduced it too much: the iframe attribute "sidebar" is missing, so the javascript wouldn't be able to access it no matter what? I added a working testcase. Actually only "document" was too much (i.e. "document.sidebar.location" instead of "sidebar.location"). I also added a second case to show that the error only occurs when the frame is named "sidebar", not with other names.
Attachment #768876 -
Attachment is obsolete: true
Attachment #768877 -
Attachment is obsolete: true
Attachment #768879 -
Attachment is obsolete: true
Flags: needinfo?(philemon-bugzilla)
Reporter | ||
Updated•11 years ago
|
Attachment #768959 -
Attachment mime type: text/plain → text/html
Attachment #768898 -
Attachment is obsolete: true
Reporter | ||
Comment 6•11 years ago
|
||
Oh, I just noticed with the new testcase, the console error (TypeError: document.sidebar is undefined @ file:...demo.html:1) doesn't happen anymore. It seems that was only part of my faulty testcase. That is, the test case fail for Firefox 22 silently, while it works with Firefox 21.
![]() |
||
Comment 7•11 years ago
|
||
Presumably the issue is that window.sidebar used to be shadowed by the frame name but now shadows the frame name... We had this issue with window.content too: see bug 857555. I guess we need to go through all the nonstandard properties on Window and do something like this...
Assignee: nobody → bobbyholley+bmo
Status: UNCONFIRMED → NEW
tracking-firefox23:
--- → ?
tracking-firefox24:
--- → ?
tracking-firefox25:
--- → ?
Ever confirmed: true
Keywords: regression
For the record :) Regression range: good=2013-03-16 bad=2013-03-17 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8f5b1f9f5804&tochange=0b052daa913c Bug 850517.
status-firefox22:
--- → affected
Updated•11 years ago
|
status-firefox23:
--- → affected
status-firefox24:
--- → affected
status-firefox25:
--- → affected
tracking-firefox22:
--- → +
Assignee | ||
Comment 9•11 years ago
|
||
bz, can you make a list of nonstandard properties I should do this for? I could try to come up with such a list, but I think you'd probably be quicker and better at it. Let me know if you don't have time.
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 10•11 years ago
|
||
I definitely can't at least for another week. :( I wonder whether it's better to just push on with WebIDL Window and mark these as ChromeOnly or something....
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #10) > I wonder whether it's better to just push on with WebIDL Window and mark > these as ChromeOnly or something.... That's certainly easier. Do we care enough about the web compat regression to want to backport a fix? If so, we need to do something like comment 9, because we sure aren't going to backport WebIDL Window. I'll defer to your (and alex's) judgement here. Thoughts?
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 12•11 years ago
|
||
I think it would be easier to answer that question if we knew the list of affected property names. I do think we should backport a fix for "sidebar".
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 13•11 years ago
|
||
Smaug, are you able to come up with such a list?
Flags: needinfo?(bugs)
Comment 14•11 years ago
|
||
I'm not familiar with the changes done in Bug 850517.
Flags: needinfo?(bugs)
Assignee | ||
Comment 15•11 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #14) > I'm not familiar with the changes done in Bug 850517. It's a question of what non-standard properties we support on the window that might conflict with named access to subframes. We previously did named subframe resolution fairly early on in nsWindowSH::NewResolve, but now it lives on the GSP. So anything from either the second half of the resolve hook or that lives on the prototype via IDL is potentially a problem. Here are the properties I can come up with: * window._content * Anything in GlobalResolve, which means anything registered via the script namespace manager for Javascript-global-property. MXR gives the following hits: ** window.sidebar ** window.external * Things from nsIDOMWindow: ** (CSSOM View stuff - is that standard?) ** window.windowRoot ** window.textZoom ** window.scrollBy{Lines,Pages}() ** window.sizeToContent() ** window.content (fixed in bug 850517) ** window.crypto ** window.pkcs11 ** window.controllers ** window.mozInnerScreen{X,Y} ** window.devicePixelRatio ** window.scrollMax{X,Y} ** window.fullScreen ** window.home() ** window.{move,resize}{To,By}() ** window.openDialog() ** window.updateCommands() ** window.find() ** window.mozPaintCount ** requestAnimationFrame stuff ** lots of onfoo event handlers, not sure which ones are specced. * Things from nsIDOMJSWindow: ** setResizable * Things from nsIDOMWindowPerformance: ** window.performance This is all just using my incomplete knowledge of what is and is not standardized. Boris, what do you think? We could fix |sidebar| (and |external|) by having GlobalResolve check for named subframes. Which (if any) props on the prototype chain do we want to handle?
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 16•11 years ago
|
||
The CSSOM view stuff is standard. find() is non-standard but commonly present in UAs, so not likely to be a performance issue. .performance is standard. move/resizeTo/By are commonly present in UAs. Most of the rest seem unlikely to collide with frame names. Let's just fix sidebar and external and call it good.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #769580 -
Flags: review?(bzbarsky)
![]() |
||
Comment 18•11 years ago
|
||
Comment on attachment 769580 [details] [diff] [review] Check for child window naming collisions before defining global properties in GlobalResolve. v1 r=me. Thank you!
Attachment #769580 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 19•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f2330ed678
Assignee | ||
Comment 20•11 years ago
|
||
Comment on attachment 769580 [details] [diff] [review] Check for child window naming collisions before defining global properties in GlobalResolve. v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 850517 User impact if declined: web compat regressions with any web page that has an iframe with name="sidebar" or name="external" and uses named subframe access (window['sidebar']). Testing completed (on m-c, etc.): Just pushed to m-i. Risk to taking this patch (and alternatives if risky): Low risk, just puts us back to the old behavior for these two property names. No alternatives. String or IDL/UUID changes made by this patch: None.
Attachment #769580 -
Flags: approval-mozilla-beta?
Attachment #769580 -
Flags: approval-mozilla-aurora?
Comment 21•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b0f2330ed678
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Reporter | ||
Comment 22•11 years ago
|
||
Do I understand correctly, that the bugfix is currently not intended to be applied to any version earlier than 25? If so, may I ask why? (the little I understand about the patch, it looks harmless enough to backport). PS: It doesn't matter for my site, because we couldn't wait for any fix anyhow and had to deploy a work-around.
Comment 23•11 years ago
|
||
The patch is requesting approval for Aurora and Beta. It means that the fix will be applied to Firefox 23 and 24 if approved.
Comment 24•11 years ago
|
||
Comment on attachment 769580 [details] [diff] [review] Check for child window naming collisions before defining global properties in GlobalResolve. v1 low risk Patch for a Fx22 regression that has a few web compat reports related to 'sidebar'. We Still have a few more beta cycles hence this is better to land asap.
Attachment #769580 -
Flags: approval-mozilla-beta?
Attachment #769580 -
Flags: approval-mozilla-beta+
Attachment #769580 -
Flags: approval-mozilla-aurora?
Attachment #769580 -
Flags: approval-mozilla-aurora+
Comment 25•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/0e435f315f3a https://hg.mozilla.org/releases/mozilla-beta/rev/106ed32b3efb
Updated•11 years ago
|
Comment 27•11 years ago
|
||
Assuming this does not need QA verification due to in-testsuite coverage. Please remove [qa-] from the whiteboard and add the verifyme keyword if this needs to be manually verified.
Whiteboard: [qa-]
Comment 28•11 years ago
|
||
Added: https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22
Keywords: dev-doc-complete
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•