[FIX]Make javascript: URI execution async

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9alpha1
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

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.
Blocks: 192128
No longer blocks: 92128
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha
Created attachment 237077 [details] [diff] [review]
Fix
Attachment #237077 - Flags: superreview?(cbiesinger)
Attachment #237077 - Flags: review?(jst)
Created attachment 237078 [details] [diff] [review]
Same as diff -w
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...
Attachment #237077 - Flags: superreview?(cbiesinger) → superreview-
Created attachment 238957 [details] [diff] [review]
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.
Attachment #237077 - Attachment is obsolete: true
Attachment #237078 - Attachment is obsolete: true
Attachment #238957 - Flags: superreview?(cbiesinger)
Attachment #238957 - Flags: review?(jst)
Attachment #238957 - Flags: approval1.7.14?
Attachment #237077 - Flags: review?(jst)
Created attachment 238958 [details] [diff] [review]
Same as diff -w
Attachment #238957 - Flags: approval1.7.14?
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" :)
Attachment #238957 - Flags: superreview?(cbiesinger) → superreview+
Created attachment 240861 [details] [diff] [review]
Updated to tip
Attachment #238957 - Attachment is obsolete: true
Attachment #238958 - Attachment is obsolete: true
Attachment #240861 - Flags: review?(jst)
Attachment #238957 - Flags: review?(jst)
Created attachment 240863 [details] [diff] [review]
Same as diff -w
Blocks: 355315
Comment on attachment 240861 [details] [diff] [review]
Updated to tip

Looks good to me, r=jst
Attachment #240861 - Flags: review?(jst) → review+
Created attachment 242455 [details] [diff] [review]
Final patch updated to all comments
Attachment #240861 - Attachment is obsolete: true
Attachment #240863 - Attachment is obsolete: true
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
could this have caused bug 356978 ?
Yeah, it did.
Depends on: 356978
Depends on: 357437
Depends on: 358660
No longer depends on: 358660
Depends on: 363594

Updated

11 years ago
Depends on: 364436

Comment 14

11 years ago
Could this have caused bug 365467?
Possible, sure.

Updated

10 years ago
Depends on: 372666
Depends on: 364028
Checked in mochitest for this.
Flags: in-testsuite+

Comment 17

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


some of these tests are failing on the linux tinderbox

Comment 18

10 years ago
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. ;)
Created attachment 260356 [details] [diff] [review]
Fix the test

Fixes the race condition that caused the orange.

Comment 20

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

Still seeing one test fail.

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.
Flags: in-testsuite+ → in-testsuite?
Depends on: 384014
Depends on: 384981
Depends on: 502806

Updated

6 years ago
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.