11577 - [regression] reading from <any frame>.location causes function to exit sliently

Status: VERIFIED FIXED

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

Description

[Alec Flett]

If I have the following code:

function foo() {
  dump("Finding frame\n");
  var destFrame = window.frames["destframe"];
  dump("Looking at location\n");
  var loc = destFrame.location;
  dump("Location is " + loc);
}

the 4th line of the function (var loc = ...) will cause this function to exit
sliently.
I have verified that destFrame is actually the correct frame (and it is an
[object Window])

I can say destFrame.location="my-url.html" and this will load a new page just
fine. It's just reading from this attribute that is busted.

Comment 1

[Alec Flett]

this is preventing the account manager from being useful.

Comment 2

[Alec Flett]

oh yes, and I forgot to add that this used to work. marking as a regression

Comment 3

[brendan-obsolete]

slamm and I were looking at this, but it seems to have been fixed in the last
day or so.  I never saw this on my build before Tuesday (my DOM idlc-generated
changes were checked in Monday night).  Yet it isn't clear what might have fixed
it, from looking at bonsai query results.  Alec, is it still happening with a
fresh pull?

/be

Comment 4

[Alec Flett]

I'll try now..

Comment 5

[Steve Lamm]

I repulled and built windows and I see the problem. I also rebuilt linux and I
continue to see the bug there.

Comment 6

[Steve Lamm]

More details on my windows build...
The bug only shows up in viewer. Apprunner executes window.location just fine.

Comment 7

[Steve Lamm]

For Windows apprunner, window.location causes silent death if it is referenced
from one of the sidebar panels. The sidebar panels are currently un-sandboxed
iframes. On windows, window.location does work in the main viewing frame which
is a sandboxed content iframe (i.e. type="content").

Comment 8

[Alec Flett]

yes! My use of .location also was from inside an IFRAME. I guess that's a little
more information..
vidur, we have not heard a word back from you... are you the right person for
this bug or should we reassign?

Comment 9

[Steve Lamm]

vidur is out this week.

Comment 10

[Alec Flett]

damn, I always seem to file bugs on people the week they're on vacation! :)

Comment 11

[tonyr]

Unfortunately, I'm not working with any code that's actually checked in yet but I
reproduced this problem by adding the following to tasksOverlay.js:
function toImport() {
	dump( "window.location: " + top.window.location + "\n");

	... code to open a dialog window...
}

Selecting "Import Utility" from the tools submenu of the Tasks menu executes the
function.  The dump string never appears and no JS code after it is executed.
I have no idea if this adds weight to the iframe info or not!

(Note: I do use window.location in an iframe which is where I first noticed the
problem.)

Comment 12

[chris hofmann]

talking with brendan we need to get this for m9 if we can.  putting on the radar

Comment 13

[Steve Lamm]

I debugged this with brendan. We found an unched call GetURL in nsLocation.cpp.
Looks like it is related to session history.

http://cvs-mirror.mozilla.org/webtools/bonsai/cvsblame.cgi?file=mozilla/dom/src/base/nsLocation.cpp&rev=1.20&mark=360#351

Did Radha or anyone else make changes to session history lately?

The call to GetHistoryIndex is returning 15 which seems unreasonably high.

Comment 14

[Steve Lamm]

Reassigning to radha. She touched the history code in nsWebShell.cpp on August 8
which is just right for when this bug appear.

Comment 15

[Steve Lamm]

radha came up with a fix. Instead of using session history to get the current
url, it will ask the webshell for its current url. It make sense to do it this
way.

Unfortunately, my code crashes now. Radha and I are looking into the second
crash. The crash only happens when I use the sidebar panels for the marketing
demo.

Comment 16

[Radha on family leave (not reading bugmail)]

OK. I looked at this with slamm. To get the href for the current page,
LocationImpl::GetHref() seems to go thro the session History which doesn't get
defined for all *non browser* windows example, the sidebar panel. A reasonable
fix to this problem is, to call mWebShell->GetURL(PRUnichar ** aURL) instead of
get the current history index and get the url for the current index which is the
same as GetUrl().

Myself and slamm tried this out in windows and linux and it works. I would like
Brendan's opinion if any on this.

Following are the diffs,
RCS file: /cvsroot/mozilla/dom/src/base/nsLocation.cpp,v
retrieving revision 1.20
diff -r1.20 nsLocation.cpp
360,361c360,361
<     mWebShell->GetHistoryIndex(index);
<     result = mWebShell->GetURL(index, &href);
---
>     //mWebShell->GetHistoryIndex(index);
>     result = mWebShell->GetURL(&href);

Slamm is seeing another crash after clearing this problem. I'm looking at it for
him

Comment 17

[vidur (gone)]

Looks good to me. I believe the history mechanism was the only way to do this
when I originally wrote the code. nsIWebShell::GetURL() was a more recent
addition.

Comment 18

[Radha on family leave (not reading bugmail)]

I'm going to put the old session history code back in webshell so that everybody
will be happy.

Comment 19

[Radha on family leave (not reading bugmail)]

*** Bug 11877 has been marked as a duplicate of this bug. ***

Comment 20

[Radha on family leave (not reading bugmail)]

*** Bug 11690 has been marked as a duplicate of this bug. ***

Comment 21

[Radha on family leave (not reading bugmail)]

Have checked in the fix approved by vidur.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 11960 has been marked as a duplicate of this bug. ***

Comment 23

[Eric Pollmann]

*** Bug 11941 has been marked as a duplicate of this bug. ***

Comment 24

[gerardok]

Alec, can you help us verify this bug? Please mark it as Verified if you agree
the resolution is correct. Thanks!

Comment 25

[Alec Flett]

yes, it works for me now, marking verified.