Skip to main content

Bug Tracker

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 ian.b.taylor@gmail.com comment:1

sorry about the bad formatting of the bug report the formatting up there should be:

scrollTop  = win.pageYOffset || docElem.scrollTop;
scrollLeft = win.pageXOffset || docElem.scrollLeft;

becomes

scrollTop  = (win && win.pageYOffset) || docElem.scrollTop;
scrollLeft = (win && win.pageXOffset) || docElem.scrollLeft;

Changed August 09, 2012 11:56PM UTC by timmywil comment:2

owner: → ian.b.taylor@gmail.com
status: newpending

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 ian.b.taylor@gmail.com comment:3

status: pendingnew

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 dmethvin comment:4

status: newpending

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 trac-o-bot comment:5

resolution: → invalid
status: pendingclosed

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!