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

Status: NEW

(Core :: XML, defect)

Description

[Phil Ringnalda (:philor)]

Judging by http://hg.mozilla.org/mozilla-central/rev/f1af6bb87895, 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 http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug428847.html?force=1 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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Phil Ringnalda (:philor)]

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?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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).

Comment 4

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

Not blocking on this.

Comment 5

[Kanchan Kumari QA]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Um... how is this possibly fixed?  Comment 4 says nothing about fixing, and bug 628966 doesn't look related....

Comment 7

[Kanchan Kumari QA]

My bad...I read previous comments and misunderstood the relation between this defect and details in bug 628966. Thanks for reopening it!

Comment 8

[Thomas Wisniewski]

Attachment: 630089-silently-ignore-cors-failures-to-load-xml-stylesheets.diff

Here's a patch which ignores CORS failures when fetching xml-stylesheets. It passes the todo'd mochitest.

A try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8f74611286d

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8783352 [details] [diff] [review]
630089-silently-ignore-cors-failures-to-load-xml-stylesheets.diff

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....

Comment 10

[Thomas Wisniewski]

Friendly review ping, since it's been a month. Peter, is there someone else you feel should tackle this instead?

Comment 11

[Peter Van der Beken [:peterv]]

I'm sort of puzzled too, what's special about CORS that we'd treat it differently from other failures?

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 8783352 [details] [diff] [review]
630089-silently-ignore-cors-failures-to-load-xml-stylesheets.diff

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.

Comment 13

[Thomas Wisniewski]

>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?

Comment 14

[Thomas Wisniewski]

(Friendly ni request ping, see comment 13).

Comment 15

[Peter Van der Beken [:peterv]]

See comment 11.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:peterv, could you please find another way to get the information or close the bug as INCOMPLETE if it is not actionable?

For more information, please visit auto_nag documentation.

Comment 18

[Stephanie Contreras]

Attachment: same problem

Thanks from laufiterloa1974.