888225 - firefox 22 breaks access to frames named 'sidebar'

Status: RESOLVED FIXED

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

Description

[philemon-bugzilla]

Attachment: demo.html

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".

Comment 1

[philemon-bugzilla]

Attachment: frame embedded in demo

Completing the test case

Comment 2

[philemon-bugzilla]

Attachment: frame loaded by javascript

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?

Comment 3

[Loic]

Attachment: Reporter's testcase

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

Comment 4

[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.
> 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.

Comment 5

[philemon-bugzilla]

Attachment: 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.

Comment 6

[philemon-bugzilla]

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

[Boris Zbarsky [:bzbarsky]]

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...

Comment 8

[Loic]

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.

Comment 9

[Bobby Holley (:bholley)]

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.

Comment 10

[Boris Zbarsky [: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....

Comment 11

[Bobby Holley (:bholley)]

(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?

Comment 12

[Boris Zbarsky [: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".

Comment 13

[Bobby Holley (:bholley)]

Smaug, are you able to come up with such a list?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

I'm not familiar with the changes done in Bug 850517.

Comment 15

[Bobby Holley (:bholley)]

(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?

Comment 16

[Boris Zbarsky [: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.

Comment 17

[Bobby Holley (:bholley)]

Attachment: Check for child window naming collisions before defining global properties in GlobalResolve. v1

Comment 18

[Boris Zbarsky [: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!

Comment 19

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f2330ed678

Comment 20

[Bobby Holley (:bholley)]

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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0f2330ed678

Comment 22

[philemon-bugzilla]

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

[Masatoshi Kimura [:emk]]

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

[bhavana bajaj [:bajaj]]

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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0e435f315f3a
https://hg.mozilla.org/releases/mozilla-beta/rev/106ed32b3efb

Comment 27

[u279076]

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.

Comment 28

[Kohei Yoshino [:kohei]]

Added: https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22