440595 - [FIX]Line numbers are incorrectly reported in onerror

Status: VERIFIED FIXED

(Core :: General, defect, P1)

Description

[William J. Edney]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008061004 Firefox/3.0
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008061004 Firefox/3.0

When a JavaScript parsing error occurs in JavaScript which has been referenced via a 'src' in a <script> tag, and you have supplied your own 'onerror' handler, the line number supplied to the onerror handler is always 0, even though the proper line number is reported in the Error Console.

See the attached files.

Reproducible: Always

Steps to Reproduce:
1. Run the test files

3. The error console shows that the line number is 7, which is correct.
Actual Results:  
The alert panel shows that the line number is 0, which is incorrect. Adding or removing code from onerrortest.js will show that the line number is always reported to the onerror handler as 0.

Expected Results:  
The alert panel should show that the error occurred on line 7, which would be the correct value. The error console shows this.

Note that Firefox 2.0 reports the line number correctly. This is something that got changed in Firefox 3.0.

Comment 1

[William J. Edney]

Attachment: The HTML file showing the bug.

Comment 2

[William J. Edney]

Attachment: The accompanying JavaScript file containing the parse error.

Comment 3

[William J. Edney]

One other thing that should be mentioned here is that if the JavaScript is 'inlined' in the HTML page (i.e. it is *not* referenced from an external file), then the line number is correctly reported.

Cheers,

- Bill

Comment 4

[Ria Klaassen (not reading all bugmail)]

Regression range is:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1206193800&maxdate=1206205379
-> Bug 402983.

Comment 5

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

Does this happen when the files are loaded from a webserver? Or only when loaded from the local file system?

If the former then the regression range sounds unlikely (though of course not impossible)

Comment 6

[William J. Edney]

Jonas -

Thanks for the quick response.

I tested it again and it only happens when they are loaded from the local file system.

Now, some of us develop, launch and load entire Web / AJAX applications and run them completely from the file system ;-), so this is of concern to us.

Thanks!

Cheers,

- Bill

Comment 7

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

Then the dependency is most likely correct. Sounds like a bad regression.

Comment 8

[rozek19]

In Fx3.0 third argument in the event handler "window.onerror" does not return a proper line number (always "0") on local external scripts (not-html but own .js file like <script src=url>).

Also, the first argument does not show the real problem but only text "Script error" (before in Fx2.0 it used to show i.e. "variable asdfj does not exist" etc.).

Comment 9

[Boris Zbarsky [:bzbarsky]]

So the issue is that we do a same-origin compare on the URI of the loading page and the loaded script.  Since each file is now a separate origin, they are not same-origin, and hence we're not allowed to leak information (such as line numbers of errors, or the actual error text) to the onerror handler.

We'd have to actually keep track of the script's origin principal (instead of just the URI) to fix this...  Well, and pass loading principals into the scriptloader too.  We have a FIXME on this in the code.

Alternately, we could try skipping the check for file:// URIs in nsJSenvironment, or replacing it with a CheckMayLoad check.

In fact, the CheckMayLoad approach may be the right one in general, instead of using CheckSameOriginURI.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 11

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

Comment on attachment 331615 [details] [diff] [review]
Like so

I *think* this is right, but please make sure jst double-checks that this doesn't revert the original intention of this code.

The only thing that is non-ideal is that you are sort of abusing the API here since you are not actually loading anything, right.

Ideally you'd want p->Subsumes(errorPrincipal) i think, but that's what the comment is about. Actually, if that is indeed the case please add that to the comment.

Comment 12

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

Though I guess it's sort of related to loading since you are handing out information related to data loaded from the particular URI...

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 331615 [details] [diff] [review]
Like so

Yeah, this does look right. sr=jst

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Test for when we have file:// mochitests

Comment 15

[Boris Zbarsky [:bzbarsky]]

Pushed changeset 3762e6244e63.  No test yet.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 331615 [details] [diff] [review]
Like so

Probably worth fixing on branch too.

Comment 17

[Mike Connor [:mconnor]]

Comment on attachment 331615 [details] [diff] [review]
Like so

Without a test, this can wait for 3.1 given the new more-restrictive branch policy.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Uh... we do have a test, and I did test this in a branch build.  We just have no way of running the test automatically on branch (or trunk, for that matter).

And of course this was a Fx3 regression that significantly complicates the job of web developers who develop against a file:// tree.

I'm OK with not taking this on branch as long as we're on the same page on the above.

Comment 20

[Tony Chung [:tchung]]

Verified fix from comment 0 on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090217 Minefield/3.2a1pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090217 Shiretoko/3.1b3pre Ubiquity/0.1.5.  

Error console and alert both show the error on line number 7.