Closed Bug 888225 Opened 7 years ago Closed 7 years ago

firefox 22 breaks access to frames named 'sidebar'


(Core :: DOM: Core & HTML, defect)

22 Branch
Not set



Tracking Status
firefox22 + wontfix
firefox23 + fixed
firefox24 + fixed
firefox25 + fixed


(Reporter: philemon-bugzilla, Assigned: bholley)



(Keywords: dev-doc-complete, regression, site-compat, Whiteboard: [qa-])


(2 files, 4 obsolete files)

Attached file demo.html (obsolete) —
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".
Attached file frame embedded in demo (obsolete) —
Completing the test case
Attached file frame loaded by javascript (obsolete) —
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
Attached file Reporter's testcase (obsolete) —
I tried with FF21 nightlies, same error.

Could you test with a clean profil, please.
Component: Untriaged → DOM
Product: Firefox → Core
Flags: needinfo?(philemon-bugzilla)
(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.
> 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.
Attached file Working testcase
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)
Attachment #768959 - Attachment mime type: text/plain → text/html
Attachment #768898 - Attachment is obsolete: true
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.
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
Ever confirmed: true
Keywords: regression
For the record :)

Regression range:

Bug 850517.
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)
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)
(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)
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)
Smaug, are you able to come up with such a list?
Flags: needinfo?(bugs)
I'm not familiar with the changes done in Bug 850517.
Flags: needinfo?(bugs)
(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)
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)
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+
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?
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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.
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 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+
Keywords: site-compat
OS: Linux → All
Hardware: x86_64 → All
Duplicate of this bug: 890533
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-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.