104361 - javascript error in navigator.js trying to view source

Status: VERIFIED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Henrik Gemal]

1. go tohttp://gemal.dk/test/2.html
2. right click on the link there and say "open in new tab"
3. then you get en error saying that the page could not be loaded.
4. select the tab with the page the didn't load
5. right click on the page and select view page source.

Error: focusedWindow.document has no properties
Source File: chrome://navigator/content/navigator.js
Line: 1039

build 20011011

Comment 1

[Peter Trudelle]

->tabbed browsing

Comment 2

[Henrik Gemal]

Error: getWebNavigation().currentURI has no properties
Source File: chrome://navigator/content/navigator.js
Line: 1061

rightclick on a link that points to a host that doesn't exist and select open in
new tab. The you get the host cannot be found alert. In the in tab right click
and select view source.

If you select open in new window instead and view source of that document, you
dont get the error. you get the source of a blank window.

Comment 3

[jag (Peter Annema)]

That's because a new window always loads a blank page before loading the actual
url. New tabs don't do that, so you end up with no page at all.

Fix 1a: make tabs load blank page first, but this will slow down new tabs
Fix 1b: detect this kind of situation and load a blank page when this happens

Fix 2: wrap this in a try/catch block and
    2a: don't open a view source window, or
    2b: open a view source window on about:blank

I think 1b would be the cleanest.

Comment 4

[Henrik Gemal]

why is there a difference is the way we load a new window and a new tab? If
loading "about:blank" is slowing us down, why load it for new window?

Comment 5

[David Hyatt]

Reassigning to new component owner.

Comment 6

[jag (Peter Annema)]

Apparently any new iframe should have an about:blank in order to be properly
initialized. The suggestion has been to use the synchronous construction of an
about:blank document for this as danm is doing elsewhere in the product,
potentially reusing that code.

Comment 7

[Dan M]

Talked with Jag. Taking.

Comment 8

[Dan M]

Attachment: initialize a lazily instantiated docshell's URL

Our frames/windows/whatevers aren't fully initialized and useable until they
have a document. Some document. Any document. That way, for instance, script
like "document.currentURI" has a document to start from. We're all set up to
create an about:blank document on demand, sparing ourselves the time if the
demand never happens.

Not in this case, though. Well, more or less. At the time you ask for View
Source it creates the document on the fly as it should (sort of; see below) but
the document is slightly malformed. The currentURI property is uninitialized,
so navigator.js BrowserViewSource() runs into an unexpected condition when it
asks for it. This patch initializes currentURI and fixes this bug.

Oh. My "sort of" comment above. Jag and I have discovered that the document
isn't being lazily instantiated early enough. It should have already been
created at least by the time the frame was displayed, but instead it waits
until the View Source request hits. That's a different bug.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 66063 [details] [diff] [review]
initialize a lazily instantiated docshell's URL

sr=jst

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 66063 [details] [diff] [review]
initialize a lazily instantiated docshell's URL

Mebbe a slightly clearer comment would help.  Even I felt stupid reading that
one, and not getting what danm's comment #8 conveys (but don't go overboard!).

r=[e-mail] either way.

/be

Comment 11

[jag (Peter Annema)]

Switcheroo fu.

Comment 12

[Dan M]

I confused Brendan! Don't know whether I should be proud or embarrassed. I'll go
with proud for now.

I was trying to make a lot of points and answer a few questions. The patch,
though, as you've already understood, was just to set the currentURI property of
the docshell after giving it a synchronously created about:blank
Frankendocument. The normal document creation algorithm does that in
CreateContentViewer (indirectly) so I've added it explicitly to
CreateAboutBlankContentViewer. Just missed a step, is all.

Comment 13

[sairuh (rarely reading bugmail)]

mass moving resolved fixed bugs pertaining to page info, view source, find in
page and open web location to [e-mail].

to find all bugspam pertaining to this, set your search string to
"SunsComingUpLikeABigBaldHead".

Comment 14

[Patty Mac]

Verified the patch.

Comment 15

[Radha on family leave (not reading bugmail)]

Is there a new website to test this bug in recent builds. 
http://gemal.dk/test/2.html is no more valid.  The patch that was checked in for
this one is causing regressions in session history. I think the patch here is
irrelevant now considering the changes that have gone into navigator.js in the
past releases. 

Comment 16

[Radha on family leave (not reading bugmail)]

Since http://gemal.dk/test/2.html itself can not be found, I opened it in a new
tab. I got the 404 server error displayed on the page (no more error dialogs). I
then right-clicked and selected "View page source". I see the page source. 

No usage of "getWebNavigation().currentURI" in navigator.js seem to be related
to "View page Sourvce". It is possible the code moved elsewhere. 

Jag,  Henrik, can you provide some data here. Thanks!

Comment 17

[Henrik Gemal]

I've put up http://gemal.dk/test/2.html again. Now I can only get:

Error: this.docShell has no properties
Source File: chrome://global/content/bindings/browser.xml#browser.webNavigation
(getter)
Line: 0

Comment 18

[Radha on family leave (not reading bugmail)]

Jag can you comment on this ....

I can now reach the page. When I click on the link to load  in a new tab, a new
tab is created and I get the dialog that says, "gemal2 could not be found....".
At this time, I also get the following error message in the console which  I
think is because the browser could not set the title for the tab. 

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "browser.currentURI has no properties" {file:
 "chrome://global/content/bindings/tabbrowser.xml#tabbrowser.setTabTitle()" line
: 9}]' when calling method: [nsIWebProgressListener::onStateChange]"  nsresult:
"0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"
  data: yes]
************************************************************

when I dismiss the dialog, the browser does not stop the load. The throbber
continues rotate and I see the message, "Resolving host www.gemal2.dk..." in the
status bar. Clicking on stop also does not stop the load. At this time, I can
not  select the "View source" from the view menu, (it is disabled) nor
right-click in the tab and "select view source".

So, it looks like there have been changes in the tab-browsing code that is
affecting this bug. In the mean time, I removed the attached patch from docshell
and tried to load blank pages in different situations. There does not seem to be
any problems. Testing more .....

Comment 19

[Radha on family leave (not reading bugmail)]

Henrik, Can you describe more on the error you see. While at the website, when
do you see that error message. 

I'm planning  to undo the changes that were proposed in this patch because

1) it is not clear that the changes proposed here are valid anymore.
2) it is causing problems in session history on pages with iframes

I would  do it as soon as the trunk opens for 1.2b development, so that I have
enough time to study any regressions. 

Comment 20

[jag (Peter Annema)]

Radha: I don't see how this patch is causing problems in session history on
pages with iframes. What the patch does is set the "about:blank" url for these
blank documents that are created if we need to show something and don't have a
real document yet. I'm pretty sure this patch is correct, but if it's causing
problems let's see how we can fix them other than backing this out.

Dan, any ideas?

Comment 21

[Radha on family leave (not reading bugmail)]

jag:  Following is the comment from bug 147364 that describes how this patch
affects session history behavior.  since the original code (in navigator.js)
that caused this bug has either vanished or moved elsewhere, I think there is
good chance this patch is not necessary. I'm not able to verify this bug in the
current codebase with this patch backed out. (please see my comments above). I
think by backing this out in the tree now, I will have enough time to play with
it, take care of my bug as well address any regressions this back out may cause. 

comments from bug 147364:
-------------------------
Just visiting the above site creates 4 entries in session history. This is
because the "about:blank" loaded in the iframes don't get registered in session
history. This is what is going on:

When the iframes are loaded with "about:blank",

1) nsDocShell::SetCurrentURI()  is called once from
CreateAboutBlankContentViewer() which sets mCurrentURI to "about:blank" .
2) The iframe then comes around for regular loading through
nsDocShell::CreateContentViewer() and since mCurrentURI is already set, in step
1)  nsDocShell::OnNewURI() decides that it is a repeat load of an existing
document and changes the loadType of the iframe to LOAD_NORMAL_REPLACE. So no
entries get created in session history for these iframes
3) Then when the onLoad Handler in the page, loads the actual url on the
iframes,  they get added to session history as top level documents. Thus simply
visiting the apge creates 4 entries in session history. (One for the main page
and 3 for the 3 iframes).  This ofcourse causes further trouble for back/forward
buttons when the user does subframe navigation in the site. 

looking to see if step 1) can be eliminated, which would allow proper creation
of the session history entries and therefore any subframe navigations...