400204 - [FIX] window.innerWidth throws exception when called on a non-displayed frame

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[(not currently active) Ted Mielczarek]

The testsuite in the URL apparently tries to access innerWidth on an iframe that isn't displayed.  This works in FF2, but now throws an exception in trunk builds.

STR:
1) Load the URL
2) Click the second run button in the list on the left (first one times out from a remote host...)
3) Test does not load, exception in Error Console:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowInternal.innerWidth]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://mavra.perilith.com/~luser/openlayers/tests/run-tests.html :: anonymous :: line 902"  data: no]

Should be easy enough to write an automated test for.

Comment 1

[Johnny Stenback (:jst)]

This is a trivial fix, we shouldn't ship with this regression IMO.

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: simple testcase

Will show "FAIL" if it gets an exception, otherwise will print the innerWidth/Height twice and then PASS.  Tries to access innerWidth/Height, then set both of them, then access them again.

I'll convert this to a mochitest as well.

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: mochitest

mochitest, in patch form.  This fails 4/6 on trunk (interestingly enough setting the values passes), and passes 6/6 on latest-1.8.

Comment 4

[Johnny Stenback (:jst)]

Interesting. I bet setting the value succeeds because it's set on a frame, and setting the innerWidht/Height on a frame is a no-op. Given that, I think would be safe to leave the setters alone for the [i]frame case, but I'll still fix them as well to match how we used to deal with this in Firefox 2 when dealing with a closed window or what not.

Comment 5

[(not currently active) Ted Mielczarek]

I could add more tests if you'd like to test closed windows or something, just give me a pointer as to what we'd be testing.

Comment 6

[Johnny Stenback (:jst)]

Attachment: Fix. Don't throw exceptions, just return 0.

This fixes both the getters and setters. I don't actually know if the setter change is testable, as closing a window will null out its mDocShell, so we'd never get to the piece of code that's changing here. I'd still prefere to make that change though for consistency with the getters if nothing else.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 285289 [details] [diff] [review]
Fix. Don't throw exceptions, just return 0.

Maybe I'm missing something, but does this patch actually init the out params to 0 if presContext is NULL?

Comment 8

[Johnny Stenback (:jst)]

The code that's changed here already does that, but it's earlier in the code than what you can see in the context in the diff.

Comment 9

[(not currently active) Ted Mielczarek]

Ah, bugzilla just doesn't play nice with your git diffs.

Comment 10

[Johnny Stenback (:jst)]

Fix checked in.

Comment 11

[(not currently active) Ted Mielczarek]

Neglected to check in that mochitest, just did it now.

Comment 12

[Johnny Stenback (:jst)]

Duh, thanks Ted!