Side navigation
#12231 closed bug (invalid)
Opened August 09, 2012 08:55PM UTC
Closed August 25, 2012 08:50AM UTC
offset.js: Null check needed in offset() function
Reported by: | ian.b.taylor@gmail.com | Owned by: | ian.b.taylor@gmail.com |
---|---|---|---|
Priority: | undecided | Milestone: | None |
Component: | unfiled | Version: | git |
Keywords: | Cc: | ||
Blocked by: | Blocking: |
Description
It's possible for the getWindow(doc) function to return null. This is happening to me inside of a Chrome extension where the document element's defaultView field is undefined (I believe for security concerns).
This means that an exception will be thrown when win.pageYOffset is called in offset.js since it lacks a null check.
Proposed solution: add a null check on win before using it on lines 35, 36 of offset.js
scrollTop = win.pageYOffset || docElem.scrollTop;
scrollLeft = win.pageXOffset || docElem.scrollLeft;
becomes:
scrollTop = (win && win.pageYOffset) || docElem.scrollTop;
scrollLeft = (win && win.pageXOffset) || docElem.scrollLeft;
This bug exists in jQuery 1.7.2 as well as current git master.
Attachments (0)
Change History (5)
Changed August 09, 2012 09:00PM UTC by comment:1
Changed August 09, 2012 11:56PM UTC by comment:2
owner: | → ian.b.taylor@gmail.com |
---|---|
status: | new → pending |
Thanks for taking the time to contribute to the jQuery project! Please provide a complete reduced test case on jsFiddle to help us assess your ticket.
Additionally, be sure to test against the jQuery Edge version to ensure the issue still exists. To get you started, use this boilerplate: http://jsfiddle.net/FrKyN/. Open the link and click to "Fork" (in the top menu) to get started.
Changed August 10, 2012 08:14AM UTC by comment:3
status: | pending → new |
---|
Sorry, I can't make a repro case in jsfiddle because the bug only occurs when jQuery is running inside of a Chrome Extension, but the proposed fix is trivial.
For more explanation of what's going on... the jQuery function getWindow will return null if a document node has no defaultView or parentWindow property.
function getWindow( elem ) { return jQuery.isWindow( elem ) ? elem : elem.nodeType === 9 ? elem.defaultView || elem.parentWindow : false; }
So under the condition where elem.nodeType === 9, but elem.defaultView and elem.parentWindow are both null or undefined, the function returns null or undefined.
This code is then called by the offset function like this, with the assumption that win will never be null or undefined:
win = getWindow( doc ); clientTop = docElem.clientTop || body.clientTop || 0; clientLeft = docElem.clientLeft || body.clientLeft || 0; scrollTop = win.pageYOffset || docElem.scrollTop; scrollLeft = win.pageXOffset || docElem.scrollLeft;
In this caase, an exception is thrown when win.pageYOffset is hit.
What I propose above is to do a null check before using win.pageYOffset and win.pageXOffset. Another solution would be to return false from getWindow in this case
function getWindow( elem ) { return jQuery.isWindow( elem ) ? elem : elem.nodeType === 9 ? elem.defaultView || elem.parentWindow || false : false; }
Either change will fix the problem.
Changed August 10, 2012 12:46PM UTC by comment:4
status: | new → pending |
---|
Unfortunately, if we don't have a way to test for this being needed, we have no way to prevent regressions. Can you provide some more information about why a Chrome extension is running that code without a window? For example, why ask for an offset if you can't get it? What's the impact on the code of not having an offset? Would it make more sense for the Chrome extension to check that in advance?
Changed August 25, 2012 08:50AM UTC by comment:5
resolution: | → invalid |
---|---|
status: | pending → closed |
Because we get so many tickets, we often need to return them to the initial reporter for more information. If that person does not reply within 14 days, the ticket will automatically be closed, and that has happened in this case. If you still are interested in pursuing this issue, feel free to add a comment with the requested information and we will be happy to reopen the ticket if it is still valid. Thanks!
sorry about the bad formatting of the bug report the formatting up there should be: