firefox 22 breaks access to frames named 'sidebar'

RESOLVED FIXED in Firefox 23

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: philemon-bugzilla, Assigned: bholley)

Tracking

({dev-doc-complete, regression, site-compat})

22 Branch
mozilla25
dev-doc-complete, regression, site-compat
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox22+ wontfix, firefox23+ fixed, firefox24+ fixed, firefox25+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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".
(Reporter)

Comment 1

4 years ago
Created attachment 768877 [details]
frame embedded in demo

Completing the test case
(Reporter)

Comment 2

4 years ago
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?

Updated

4 years ago
Attachment #768876 - Attachment mime type: text/plain → text/html

Comment 3

4 years ago
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

Updated

4 years ago
Component: Untriaged → DOM
Product: Firefox → Core

Updated

4 years ago
Flags: needinfo?(philemon-bugzilla)

Updated

4 years ago
Blocks: 850517
(Reporter)

Comment 4

4 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

4 years ago
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.
Attachment #768876 - Attachment is obsolete: true
Attachment #768877 - Attachment is obsolete: true
Attachment #768879 - Attachment is obsolete: true
Flags: needinfo?(philemon-bugzilla)
(Reporter)

Updated

4 years ago
Attachment #768959 - Attachment mime type: text/plain → text/html

Updated

4 years ago
Attachment #768898 - Attachment is obsolete: true
(Reporter)

Comment 6

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

Comment 8

4 years ago
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

4 years ago
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox25: --- → affected
tracking-firefox22: --- → +
tracking-firefox23: ? → +
tracking-firefox24: ? → +
tracking-firefox25: ? → +
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)
Created attachment 769580 [details] [diff] [review]
Check for child window naming collisions before defining global properties in GlobalResolve. v1
Attachment #769580 - Flags: review?(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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f2330ed678
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?
https://hg.mozilla.org/mozilla-central/rev/b0f2330ed678
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox25: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Reporter)

Comment 22

4 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.
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/0e435f315f3a
https://hg.mozilla.org/releases/mozilla-beta/rev/106ed32b3efb
status-firefox22: affected → wontfix
status-firefox23: affected → fixed
status-firefox24: affected → fixed

Updated

4 years ago
Keywords: site-compat
OS: Linux → All
Hardware: x86_64 → All

Updated

4 years ago
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-]
Added: https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22
Keywords: dev-doc-complete
You need to log in before you can comment on or make changes to this bug.