Last Comment Bug 888225 - firefox 22 breaks access to frames named 'sidebar'
: firefox 22 breaks access to frames named 'sidebar'
Status: RESOLVED FIXED
[qa-]
: dev-doc-complete, regression, site-compat
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 22 Branch
: All All
: -- normal (vote)
: mozilla25
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
: 890533 (view as bug list)
Depends on:
Blocks: 850517
  Show dependency treegraph
 
Reported: 2013-06-28 04:23 PDT by philemon-bugzilla
Modified: 2013-10-28 16:12 PDT (History)
11 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
fixed
+
fixed
+
fixed


Attachments
demo.html (162 bytes, text/html)
2013-06-28 04:23 PDT, philemon-bugzilla
no flags Details
frame embedded in demo (26 bytes, text/plain)
2013-06-28 04:25 PDT, philemon-bugzilla
no flags Details
frame loaded by javascript (24 bytes, text/plain)
2013-06-28 04:31 PDT, philemon-bugzilla
no flags Details
Reporter's testcase (170 bytes, text/html)
2013-06-28 05:32 PDT, Loic
no flags Details
Working testcase (372 bytes, text/html)
2013-06-28 07:53 PDT, philemon-bugzilla
no flags Details
Check for child window naming collisions before defining global properties in GlobalResolve. v1 (2.34 KB, patch)
2013-06-30 22:08 PDT, Bobby Holley (:bholley) (busy with Stylo)
bzbarsky: review+
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description philemon-bugzilla 2013-06-28 04:23:56 PDT
Created attachment 768876 [details]
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 2013-06-28 04:25:08 PDT
Created attachment 768877 [details]
frame embedded in demo

Completing the test case
Comment 2 philemon-bugzilla 2013-06-28 04:31:15 PDT
Created attachment 768879 [details]
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 2013-06-28 05:32:43 PDT
Created attachment 768898 [details]
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 2013-06-28 07:13:25 PDT
(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 2013-06-28 07:53:01 PDT
Created attachment 768959 [details]
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 2013-06-28 08:00:46 PDT
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 [:bz] 2013-06-28 08:05:17 PDT
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 2013-06-28 08:15:57 PDT
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) (busy with Stylo) 2013-06-28 22:20:54 PDT
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 [:bz] 2013-06-28 22:26:41 PDT
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) (busy with Stylo) 2013-06-29 08:34:18 PDT
(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 [:bz] 2013-06-29 22:02:25 PDT
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) (busy with Stylo) 2013-06-29 22:09:26 PDT
Smaug, are you able to come up with such a list?
Comment 14 Olli Pettay [:smaug] 2013-06-30 08:26:50 PDT
I'm not familiar with the changes done in Bug 850517.
Comment 15 Bobby Holley (:bholley) (busy with Stylo) 2013-06-30 18:05:04 PDT
(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 [:bz] 2013-06-30 18:55:22 PDT
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) (busy with Stylo) 2013-06-30 22:08:56 PDT
Created attachment 769580 [details] [diff] [review]
Check for child window naming collisions before defining global properties in GlobalResolve. v1
Comment 18 Boris Zbarsky [:bz] 2013-07-02 16:15:25 PDT
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) (busy with Stylo) 2013-07-02 17:36:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f2330ed678
Comment 20 Bobby Holley (:bholley) (busy with Stylo) 2013-07-02 17:37:45 PDT
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] 2013-07-03 11:33:16 PDT
https://hg.mozilla.org/mozilla-central/rev/b0f2330ed678
Comment 22 philemon-bugzilla 2013-07-04 01:52:06 PDT
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] 2013-07-04 02:16:59 PDT
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] 2013-07-04 13:09:29 PDT
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 26 Loic 2013-08-08 11:43:26 PDT
*** Bug 890533 has been marked as a duplicate of this bug. ***
Comment 27 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-09-18 15:50:53 PDT
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.

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