Closed Bug 373181 Opened 18 years ago Closed 18 years ago

onload doesn't fire for XHTML documents that contain a script tag (Firefox's RSS preview is broken)

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
1.8 Branch
Type:
defect
Priority:
Not set
Severity:
blocker

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: tracy, Assigned: sicking)

References

(
URL
)

Details

(5 keywords)

Attachments

(4 files)

screen shot of broken Viewing Feed page
18.31 KB, image/png
Details
testcase
268 bytes, application/xml
Details
Patch to fix
4.33 KB, patch
jst
: review+
jst
: superreview+
sicking
: approval1.8.1.3+
sicking
: approval1.8.0.11+
Details | Diff | Splinter Review
add mochikit test
1.53 KB, patch
sicking
: review+
Details | Diff | Splinter Review
tested with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3pre) Gecko/20070308 BonEcho/2.0.0.3pre (also tested and failed on Linux and Mac builds nightly BE builds) Steps to reproduce 1) Got to http://del.icio.us 2) Click on the RSS feed icon in the location bar 3) In the resulting Viewing Feed page, click on the Subscribe Now button Tested results: Nothing happens. This page/subscribe dialog also looks incomplete. I'll attach a screen shot shortly Expected results: An add feed dialog appears which allows to add the feed to our bookmarks
Flags: blocking1.8.1.3?
Flags: blocking1.8.1.3? → blocking1.8.1.3+
The regression window for this is between builds of ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2007-03-06-04-mozilla1.8 (working RSS subscription) and ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2007-03-07-04-mozilla1.8 (broken RSS subscription)
Flags: blocking1.8.1.3+ → blocking1.8.1.3?
seems to be a 1.8.1.3 branch regression only, because wfm on Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/2007030804 Minefield/3.0a3pre
Definitely a blocker until we resolve.
Flags: blocking1.8.1.3? → blocking1.8.1.3+
Backing out the patch for bug 371576 fixes this.
Blocks: 371576
Keywords: regression
Attached file testcase —
The presence of the script tag seems to prevent onload from firing.
Assignee: nobody → general
Component: RSS Discovery and Preview → DOM
Product: Firefox → Core
QA Contact: rss.preview → ian
Version: 2.0 Branch → 1.8 Branch
I can reproduce this on the 1.8.0 and 1.8 branches, but not on the trunk.
Summary: RSS feed Subscribe Now (Viewing Feed) doesn't subscribe to feed → onload doesn't fire for XHTML documents that contain a script tag (Firefox's RSS preview is broken)
Assignee: general → jonas
Flags: blocking1.8.0.11+
Flags: in-testsuite?
The problem is that in bug 371576 i made resuming the parser after it was blocked async. This was a bad idea for the branch. What happens is this <html> <head> <script src="data:text/plain,"/> </head> <body onload="..."/> </html> When we hit the <script> we fire off a new network load and block the parser. Then we sit and wait for that load to finish. In the meantime the load for the document finishes and is removed from the loadgroup. Once the network load for the script finishes we execute the script and tell the sink to resume parsing. What used to happen: We would on the current stack resume the parsing and once we hit the end of the document return. We'd then unwind and remove the script-load from the loadgroup which would trigger onload. What happens with the patch from bug 371576: We'd fire an event to resume parsing asyncronously We'd then unwind and remove the script-load from the loadgroup which would trigger onload. We finally finish parsing off the event and at that point parse the <body> element and the onload attribute. In other words. We still do fire onload. We just do it before we parse the body-onload attribute. Patch coming up.
Attached patch Patch to fix — — Splinter Review
This is simply a backout of the contentsink changes from bug 371576.
Attachment #257890 - Flags: superreview?(bzbarsky)
Attachment #257890 - Flags: review?(bzbarsky)
Attachment #257890 - Flags: approval1.8.1.3?
Attachment #257890 - Flags: approval1.8.0.11?
Comment on attachment 257890 [details] [diff] [review] Patch to fix r+sr=jst
Attachment #257890 - Flags: superreview?(bzbarsky)
Attachment #257890 - Flags: superreview+
Attachment #257890 - Flags: review?(bzbarsky)
Attachment #257890 - Flags: review+
Comment on attachment 257890 [details] [diff] [review] Patch to fix got r=preed over irl
Attachment #257890 - Flags: approval1.8.1.3?
Attachment #257890 - Flags: approval1.8.1.3+
Attachment #257890 - Flags: approval1.8.0.11?
Attachment #257890 - Flags: approval1.8.0.11+
This was checked in on branches yesterday. Keeping the bug open to investigate if trunk needs work too (though I don't think it does)
verified fixed for 1.8.1.3 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.3pre) Gecko/2007030903 BonEcho/2.0.0.3pre RSS Feed Preview and subscribe is now working fine. Also with Lvie Bookmarks and RSS Applications.
Keywords: fixed1.8.1.3 → verified1.8.1.3
also verified fixed for 2.0.0.3 RC1 Build identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.3) Gecko/2007030919 Firefox/2.0.0.3 RSS Feed Preview works fine
(In reply to comment #5) > Backing out the patch for bug 371576 fixes this. > Make sure we don't forget about bug 371321...
Yeah, i tested that bug before checking in.
since jonas is away, maybe jst can answer this question - the RSS feed functionality is different on the 1.8.0 branch (no feed preview UI). Testing with 1.5.0.10 I don't see any brokenness (I am able to add a feed by clicking on the RSS Icon in the URL bar), so I am trying to understand how the patch affects the 1.8.0 branch in terms of verification. Thanks.
The Firefox 1.5.0.x chrome doesn't include a way to test this bug. Testing the attached testcase should suffice for verifying the bug there, I think.
The testcase passes fine in the 1.5.0.11 candidate build, but it does when I run it in Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.10) Gecko/20070216 Firefox/1.5.0.10 as well. If that is expected I can verify the bug. (In reply to comment #20) > The Firefox 1.5.0.x chrome doesn't include a way to test this bug. Testing the > attached testcase should suffice for verifying the bug there, I think. >
(In reply to comment #21) > The testcase passes fine in the 1.5.0.11 candidate build, but it does when I > run it in Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.10) > Gecko/20070216 Firefox/1.5.0.10 as well. That 1.5.0.10 build doesn't include the patch for bug 371576 (which caused this bug), so the fact that it passes is expected. To test a failing build, you need a 1.8.0 branch build from 2007-03-07. This bug was only on the branches for ~1 day, it wasn't a regression from 1.5.0.10.
verified on the 1.8.0 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.11) Gecko/20070312 Firefox/1.5.0.11. I used Gavin's testcase in Comment 6 to verify the bug. I also checked back and did see the breakage on the 20070307 build, and can verify that the test case does fail on that build. Thanks for the clarification, I am adding the verified keyword to this bug.
Keywords: fixed1.8.0.11 → verified1.8.0.11
Should this bug not become resolved->fixed?
I'd like to check that this for sure doesn't break on trunk first. Just testing seems to indicate that it works, but I'd like to make sure. If someone could write up some mochikit tests that would help a lot.
Adds a MochiKit test for this bug. I ran the test on trunk (pass), latest 1.8 branch (pass), and a build from the regression window (fail).
Attachment #261795 - Flags: review?(jonas)
Attachment #261795 - Flags: review?(jonas) → review+
Checked in the test.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: