Open Bug 630089 Opened 9 years ago Updated 4 months ago

Cross-site XHR regressed bug 428847, showing an error message for blocked stylesheet loads


(Core :: XML, defect)

Not set



Tracking Status
blocking2.0 --- -


(Reporter: philor, Assigned: wisniewskit, NeedInfo)



(Keywords: regression)


(1 file)

Judging by, it was cross-site XHR that took us from having a blocked xslt stylesheet load just quietly fail to having a Yellow Screen of Death. Test case is in the tree - either uncomment the commented out bits of or change the todo to an is if I've landed my patch in bug 628966. Expected is that the mochitest frame will be blank, actual is a "Error loading stylesheet: A network error occurred
loading an XSLT stylesheet" YSoD.

Nomming for blocking2.0 because bug 428847 blocked 1.9, when we didn't want to break XHTML pages that have bogus PIs, like we're apparently about to do with 2.0.
Jonas, can you take a look?

That said, this didn't regress the exact issue bug 428847 was actually about, as far as I can tell.  In particular, in 3.6 (and 3.5, and 3.0) if your XSLT PI points to a nonexistent file you get a YSOD.  Same thing if your XLST PI points to a file that's not allowed to be loaded in 4.0.

Am I just missing something?
Assignee: nobody → jonas
Huh, I took bug 428847 at face value for what it and the test said it was about, which was "never mind whether there's a file there or not, if you aren't allowed to load it don't YSoD." So what was it actually about? Or is the change that before we wouldn't load it, so we wouldn't know it doesn't exist, but now we're looking for cross-origin headers so we do load it and give it the treatment we used to give same-origin 404s?
Bug 428847 fixed cases in which we threw due to asyncOpen failing.  It didn't fix other network failure cases (and in particular, looks like didn't fix some of the testcases attached to that bug).
Not blocking on this.
blocking2.0: ? → -
Considering the age of bug and it seems to be fixed as comment#4 and bug 628966. Closing the bug as resolved:fixed. Feel free to reopen if this is still an issue.
Closed: 4 years ago
Resolution: --- → FIXED
Um... how is this possibly fixed?  Comment 4 says nothing about fixing, and bug 628966 doesn't look related....
Flags: needinfo?(kkumari)
Resolution: FIXED → ---
My bad...I read previous comments and misunderstood the relation between this defect and details in bug 628966. Thanks for reopening it!
Flags: needinfo?(kkumari)
Here's a patch which ignores CORS failures when fetching xml-stylesheets. It passes the todo'd mochitest.

A try run seems fine:

If this seems like a reasonable fix, then I can also add a test-specific XSL file to the tree, rather than having it try to load one from another test.
Assignee: jonas → wisniewskit
Attachment #8783352 - Flags: review?(jst)
Comment on attachment 8783352 [details] [diff] [review]

Peter or Jonas (with the latter not really an option now, I guess) would be better reviewers here in terms of understanding the semantics of this stuff.  For example, why should CORS failures be treated differently from 404 response or server connection reset in this code?

Note also that NS_ERROR_DOM_BAD_URI does NOT mean CORS failure.  It can happen for other reasons....
Attachment #8783352 - Flags: review?(jst) → review?(peterv)
Friendly review ping, since it's been a month. Peter, is there someone else you feel should tackle this instead?
Flags: needinfo?(peterv)
I'm sort of puzzled too, what's special about CORS that we'd treat it differently from other failures?
Flags: needinfo?(peterv)
Comment on attachment 8783352 [details] [diff] [review]

You shouldn't change the existing testcase but add new ones. And as bz already pointed out, NS_ERROR_DOM_BAD_URI is more generic than CORS failures.
Attachment #8783352 - Flags: review?(peterv) → review-
>NS_ERROR_DOM_BAD_URI is more generic than CORS failures.

Hmm. How should I go about distinguishing that it's a CORS failure? Perhaps also find a way to check that that the channel's loadInfo->GetTainting() == LoadTainting::CORS ?

>You shouldn't change the existing testcase but add new ones.

I take it that that I should at least change the existing testcase by getting rid of the todo(), since that's what the new test will be handling?
Flags: needinfo?(peterv)
(Friendly ni request ping, see comment 13).
Flags: needinfo?(peterv) → needinfo?(wisniewskit)
You need to log in before you can comment on or make changes to this bug.