318473 - [FIX]Page with <script type="[whitespace]"> in it never finishes loading

Status: RESOLVED FIXED

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

Description

[j.j.]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

<script type=" ">
Any script with whitespace as value for type seems to hang the browser.
FF 1.0 did not hang and ignored those scripts.


Reproducible: Always

Steps to Reproduce:
1. see testcase1

Actual Results:  
FF 1.5 looks like never sops loading.
After clicking "STOP" no content displays.

Expected Results:  
displaying "TEXT TEXT TEXT"

Comment 1

[j.j.]

Attachment: testcase1

Comment 2

[Adam Guthrie]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051130 Firefox/1.6a1

Confirming. I see this in a debug build:

WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file /moz/mozilla/content/base/src/nsScriptLoader.cpp, line 414

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Probably closely related to bug 306502 (via bug 62485).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Not at all related.  First of all, this is not a hang.  A hang is when the program doesn't respond.

Brendan, this is all yours -- the patch for bug 270553 (revision 1.67 of nsScriptLoader.cpp) makes nsScriptLoader::ProcessScriptElement return an error if GetParameter() does (as it does in this case, since there is no non-whitespace text in the "type" attribute).  Note that all the early returns from ProcessScriptElement except the ones you added in that patch call FireErrorNotification() so that the parser (which blocked on the script) will unblock.  Your code here needs to do the same...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Something like this?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #4)
> Not at all related.  First of all, this is not a hang.  A hang is when the
> program doesn't respond.

My bad, I hadn't actually tested since I was otherwise busy and couldn't hang, and apparently I missed the part of comment 0 that mentions UI interaction after reproducing.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 204723 [details] [diff] [review]
Something like this?

Thanks for patching -- you should take the bug, if you want to commit the fix, even though it's all my fault :-/.

I don't see the point in hiding return in a macro that sets rv before its last use.  Why not keep the code pellucid and let the auto-helper do its thing without a hint of (non-existent) cleanup magic in the macro?  We don't need to future-proof against a dystopia where the auto-helper or others like it aren't enough, and the macro has to grow hair ;-).

/be

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I don't see the point in hiding return in a macro that sets rv before its last
> use. 

I just got tired of typing that code over and over.  I guess I can just do it, though, and ditch the macro...

Comment 9

[Brendan Eich [:brendan]]

bz and I talked on IRC, where I argued that auto helpers must be full-auto to be useful for future-proofing.  Since all exit paths must cleanup, I proposed using a subroutine for the guts, and having the caller do the mandatory notifying.

/be

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: So more like this

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 204731 [details] [diff] [review]
So more like this

Looks good to me with the added assertion we talked about on IRC (that we never notify NS_OK).

/be

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 204731 [details] [diff] [review]
So more like this

A lot of that code could (should) be converted to use NS_ENSURE_XXX rather then |if|

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 204731 [details] [diff] [review]
So more like this

bz asked me to list which ifs i was referring to. I'd basically 


>   // We need a document to evaluate scripts.
>   if (!mDocument) {
>-    return FireErrorNotification(NS_ERROR_FAILURE, aElement, aObserver);
>+    return NS_ERROR_FAILURE;
>   }

Convert

>   // Check whether we should be executing scripts at all for this document
>   if (!mDocument->IsScriptEnabled()) {
>-    return FireErrorNotification(NS_ERROR_NOT_AVAILABLE, aElement, aObserver);
>+    return NS_ERROR_NOT_AVAILABLE;
>   }

This one cold be combined with the |!mEnabled| test a few lines up.

>   // Create a request object for this script
>   nsRefPtr<nsScriptLoadRequest> request = new nsScriptLoadRequest(aElement, aObserver, jsVersionString, hasE4XOption);
>   if (!request) {
>-    return FireErrorNotification(NS_ERROR_OUT_OF_MEMORY, aElement, aObserver);
>+    return NS_ERROR_OUT_OF_MEMORY;
>   }

I like to see these using NS_ENSURE, but i know other people don't :)

>     if (!docPrincipal) {
>-      return FireErrorNotification(NS_ERROR_UNEXPECTED, aElement, aObserver);
>+      return NS_ERROR_UNEXPECTED;
>     }

Convert

>     rv = nsContentUtils::GetSecurityManager()->
>       CheckLoadURIWithPrincipal(docPrincipal, scriptURI,
>                                 nsIScriptSecurityManager::ALLOW_CHROME);
> 
>     if (NS_FAILED(rv)) {
>-      return FireErrorNotification(rv, aElement, aObserver);
>+      return rv;
>     }

Convert

>     // If we've got existing pending requests, add ourselves
>     // to this list.
>     if (mPendingRequests.Count() > 0) {
>       request->mWasPending = PR_TRUE;
>-      mPendingRequests.AppendObject(request);
>+      if (!mPendingRequests.AppendObject(request)) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
>     }

Same as the other OOM.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed.  I'm still not sure what 1.8.0.1 vs 1.8.1 are really shaping up to be, so requesting blocking so we remember to make a call on this....

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 204757 [details] [diff] [review]
Updated to comments

Since we're making compat fixes on 1.8.0.1, we should take this.  I would have requested approval weeks ago if this had been clear then...

Comment 17

[Brendan Eich [:brendan]]

This should be reconsidered -- it's a good fix, smaller than the diff makes it look, to avoid crapping out without cleaning up, after starting something that needs cleanup.

/be

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 204757 [details] [diff] [review]
Updated to comments

a=dveditz for drivers

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to 1.8 branch

Comment 20

[Boris Zbarsky [:bzbarsky]]

Fixed on both branches.

Comment 21

[Jay Patel [:jay]]

v.fixed on 1.8.0.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1, no hang with j.j.'s testcase in comment #1, page text loads as expected.