351633 - [FIX]Make javascript: URI execution async

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

My testing indicates that's what other browsers (Safari, Opera, IE6/Windows, Konqueror) do.

See testcases at http://web.mit.edu/bzbarsky/www/testcases/javascript-url/ (except dummy* and fail.html and pass.html).

The patch coming up fixes for me bug 192128, bug 195562, bug 344874, bug 344882, bug 344996, bug 343940, bug 305546.

It does not regress bug 343850 or bug 257875 or bug 139798.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

Comment 3

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

Comment on attachment 237077 [details] [diff] [review]
Fix

(I've only read the -w patch. I trust you to get the indentation right :) )

+    // we're just running under the event loop, so we shouldn't propagate JS
+    // exceptions out of here.
+    ::JS_ReportPendingException((JSContext*)scriptContext->GetNativeContext());

So, that's not quite true, since (chrome) JS can call open() on the channel,
which calls this basically directly. Is that a problem?

 nsJSChannel::GetStatus(nsresult *aResult)
 {
     // We're always ok. Our status is independent of our underlying
     // stream's status.
-    *aResult = NS_OK;
+    *aResult = mStatus;

Well that comment is lying now, isn't it :)

 nsJSChannel::Open(nsIInputStream **aResult)
 {
-    return InternalOpen(PR_FALSE, nsnull, nsnull, aResult);
+    NS_ENSURE_STATE(mIOThunk);

How can mIOThunk be null here?

+    nsCOMPtr<nsIRunnable> ev =
+        new nsRunnableMethod<nsJSChannel>(this, &nsJSChannel::EvaluateScript);

Need to nullcheck this.

+    if (NS_FAILED(rv)) {
+        loadGroup->RemoveRequest(this, nsnull, rv);
+        mIsActive = PR_FALSE;
+    }

Should probably null out mContext here.

+    // Note that we want to be in the loadgroup when we do this so we find out
+    // if we get canceled.

I don't understand this comment. Cancel() gets called if this channel gets
cancelled. And, channels are supposed to be in their loadgroup all the time
while they are pending...


Is it just me or do you never set mOpenedStreamChannel to true?

+    // Otherwise, mStreamChannel will take over notifications.

Perhaps this is slightly misleading, since it is the nsJSChannel which
actually calls the listener's methods...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to sr comments

> So, that's not quite true, since (chrome) JS can call open() on the channel,

Indeed.  Modified comment accordingly.

> Well that comment is lying now, isn't it :)

Good catch.  Fixed.  ;)

> How can mIOThunk be null here?

I can't.  Good catch, and fixed.

> Need to nullcheck this.

Actually, no.  NS_DispatchToCurrentThread is documented to null-check stuff so callers don't have to worry about it.  We use this pattern all over.

> Should probably null out mContext here.

Yep.  And mListener.  Fixed.

> I don't understand this comment.

I tried to make it clearer.  Let me know if I did.

> Is it just me or do you never set mOpenedStreamChannel to true?

Good catch!  Fixed.

> Perhaps this is slightly misleading

Indeed.  I tried to make this clearer.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

Comment 6

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

Comment on attachment 238957 [details] [diff] [review]
Updated to sr comments

+    // script returns it).
+    
+    if (NS_SUCCEEDED(mStatus)) {

please don't add trailing whitespace :)


// Otherwise, mStreamChannel will call OnStartRequest and OnStopRequest on
// us, so we'll be sure to call them on our listener.

OK, but now the if above has both a then and an else-clause, so I'd just move the comment there and remove the "otherwise" :)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 240861 [details] [diff] [review]
Updated to tip

Looks good to me, r=jst

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Final patch updated to all comments

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 12

[Peter van der Woude [:Peter6]]

could this have caused bug 356978 ?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yeah, it did.

Comment 14

[[e-mail]]

Could this have caused bug 365467?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Possible, sure.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Checked in mochitest for this.

Comment 17

[Robert Sayre]

(In reply to comment #16)
> Checked in mochitest for this.


some of these tests are failing on the linux tinderbox

Comment 18

[Robert Sayre]

we agreed to clobber the linux test box to reproduce this failure, but it didn't respond, so I'm leaving this checked-in. Surely someone will checkin to the closed tree with orange. ;)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix the test

Fixes the race condition that caused the orange.

Comment 20

[Robert Sayre]

(In reply to comment #19)
> 
> Fixes the race condition that caused the orange.

Still seeing one test fail.

Comment 21

[Boris Zbarsky [:bzbarsky]]

About half the load-stopping tests are now commented out or otherwise not working.  Setting back to in-testsuite? -- someone needs to figure out how to test this stuff from mochitest.  I haven't been able to.