Bug Tracker

Opened 7 years ago

Closed 7 years ago

#12231 closed bug (invalid)

offset.js: Null check needed in offset() function

Reported by: ian.b.taylor@… Owned by: ian.b.taylor@…
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 scrollLeft = win.pageXOffset
docElem.scrollTop;
docElem.scrollLeft;

becomes:

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

This bug exists in jQuery 1.7.2 as well as current git master.

Change History (5)

comment:1 Changed 7 years ago by ian.b.taylor@…

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;

comment:2 Changed 7 years ago by timmywil

Owner: set to ian.b.taylor@…
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.

comment:3 Changed 7 years ago by ian.b.taylor@…

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.

comment:4 Changed 7 years ago by dmethvin

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?

comment:5 Changed 7 years ago by trac-o-bot

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!

Note: See TracTickets for help on using tickets.