257875 - invalid javascript keeps logo animating

Status: RESOLVED FIXED

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

Description

[Edwin Groothuis]

User-Agent:       Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.2) Gecko/20040814
Build Identifier: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.2) Gecko/20040814

When I open a new tab, enter some incorrect javascript in the address bar
(javascript:blaat for example), the logo stays being animated.
When I switch to a different tab, the logo stays being animated.
When I close the tab with the offending javascript, the logo stays animated.
When I load a page in a different tab, the logo stops being animated.

Reproducible: Always
Steps to Reproduce:
1. open a new tab
2. enter invalid javascript (javascript:blaat) in the address bar
3. switch tabs / close tab



Expected Results:  
I expected the logo to stop animating the moment the data in the addressbar was
parsed and processed.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Not a JS engine issue.

Comment 2

[Brendan Eich [:brendan]]

I hope nothing regressed -- can't reproduce in older Firefoxes.

/be

Comment 3

[Johnny Stenback (:jst)]

Hmm, I do see this in an aviary branch build from today (linux).

Comment 4

[Brendan Eich [:brendan]]

This could indicate something stinky, like a fix missing from aviary-branch. 
Nominating for 1.0.

/be

Comment 5

[Johnny Stenback (:jst)]

This looks like a problem with OnStatusChange() notifications being dropped in
this case. Note that this appears to only happen the first time something is
loaded in a new tab, the only way I can reproduce this is to open a new tab and
as the first thing, load a bogus javascript: URL. We don't get into
onStatusChange() in browser.js with a STATE_STOP state, which appears to be the
code that should turn off the throbber. I'm too sleep deprived to figure out why
at the moment...

Comment 6

[Uri Bernstein]

See also bug 259794, which might be related.
I suggest changing "OS" to "All", as I'm seeing it on Win98.

Also, any particular reason why the component is "DOM level 0"?

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

No owner, no patch. Make this block when there's something along those lines. 

Comment 8

[Johnny Stenback (:jst)]

Attachment: Possible fix.

This does fix this problem, but I don't fully understand why this is needed,
nor why this is only a problem when loading into a tab, and not when loading
into the main document (w/ no other tabs open). This makes sure we post the
proper nsIWebProgress notifications in the case where we're loading a
javascript: URI that doesn't return a value.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 161222 [details] [diff] [review]
Possible fix.

What do you guys think?

Comment 10

[Boris Zbarsky [:bzbarsky]]

just don't know this webprogress stuff that well (and I certainly don't know
what tabbrowser does with it).

biesi, do you?  If so, could you review?

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

hm, I suspect the javascript: channel is violating some invariant somewhere.
especially, I'm concerned that it has some other channel (mStreamChannel) manage
its loadgroup, I suspect that may be related to this bug.

that GetStatus always returns NS_OK also sounds like a bad idea.

hm, it also cancels mStreamChannel in some cases even though asyncOpen failed...

I'll look some more at this later today...

Comment 12

[Brendan Eich [:brendan]]

Any differences between trunk and branches in nsJSProtocolHandler.cpp and/or
nsDocLoader.cpp?

/be

Comment 13

[Johnny Stenback (:jst)]

I only see cleanup and deCOMtamination differences between trunk and aviary
branch for those files.

Comment 14

[Darin Fisher]

Attachment: Alternate patch

Johnny and I cooked up an alternate patch for this bug.  It basically involves
suppressing the loadgroup notifications (and hence the webprogresslistener
notifications) for the AddRequest and RemoveRequest calls made around
EvaluateScript.  We only add ourselves to the loadgroup there so that we can
intercept cancelation, which corresponds to the JS triggering
nsDocShell::LoadURI.  By temporarily setting LOAD_BACKGROUND, we still get the
cancelation notification, but we avoid tickling the throbber.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 163641 [details] [diff] [review]
Alternate patch

Yeah, much better! r+sr=jst

Comment 16

[Darin Fisher]

fixed-on-trunk