Closed Bug 444322 Opened 16 years ago Closed 16 years ago

Firefox 3 onload and DOMContentLoaded event firing before the page is fully loaded.

Categories

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

All
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: kurt, Assigned: mrbkap)

References

()

Details

(Keywords: fixed1.9.1, regression, verified1.9.0.6)

Attachments

(4 files, 3 obsolete files)

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

We were finally able to isolate the behavior to a few lines of html and javascript (see the welcome.jsp discussed later). The faulty behavior occurs more often when the complexity of the page increases, however the welcome.jsp demonstrates the problem quite well if you let it run for a while. After the welcome.jsp is loaded, the javascript will load the return_to_welcome.jsp, which is identical, but it redirects back to welcome.jsp. When the resource loading goes bad it will throw up an alert saying it cannot find the clock widget. The clock widget is a widget provided by dojo. We are using dojo 0.4.

We added logging to the javascript and this is displayed on the page when resource loading goes bad:

WELCOME
LOGGER RESULTS
BEGIN dojo.js.uncompressed.js
Adding dj_load_init() to DOMContentLoaded Event
END dojo.js.uncompressed.js
-------------------
BEGIN INLINE SCRIPT
-------------------
Before dojo.require(dojo.widget.Clock)
Begin dj_load_init(). (Triggered from EVENT: domcontentloaded)
dojo.hostenv.makeWidgets() Called
dojo.hostenv.makeWidgets() Ended
Begin dj_load_init(). (Triggered from EVENT: load)
After dojo.require(dojo.widget.Clock)
Before dojo.addOnLoad(init)
init() Called
EXPECTED RESULTS
-------------------------------------------------------------------------
BEGIN dojo.js.uncompressed.js
Adding dj_load_init() to DOMContentLoaded Event
END dojo.js.uncompressed.js
-------------------
BEGIN INLINE SCRIPT
------------------
Before dojo.require(dojo.widget.Clock)
After dojo.require(dojo.widget.Clock)
Before dojo.addOnLoad(init)
After dojo.addOnLoad(init)
-------------------
END INLINE SCRIPT
-------------------
Begin dj_load_init(). (Triggered from EVENT: domcontentloaded)
dojo.hostenv.makeWidgets() Called
dojo.hostenv.makeWidgets() Ended
init() Called
init() Finished

You can run the example at:

http://www.vodafonegroup.eu:8080/jsp-ex ... elcome.jsp

If you want I can tar up the war file, or even the complete tomcat.

The logging clearly shows that the onLoad event is fired *BEFORE* all the javascript has been loaded. The issue is most easily reproducible on windows (we used both win2k and xp in a VM), but we've also seen it on OSX. However for the example code provided we could only make it happen on windows. All runs fine in FF2, or IE6+. We used a clean FF3 with no plugins.

BTW it is key to use a jsp and *not* static HTML, or else the browser figures out that it can cache the entire page and the issue will not surface. Sometimes the issue will issue on the first load, sometimes it will take a few minutes..

Another maybe related issue is that we sometimes see that CSS is not loaded. I also actually see this quite often when I go to www.cnn.com with ff3.

BTW in FF2 you can actually see the clock widget on the welcome.jsp, not so in ff3.

You can actually see from the bad results (the first set) that the dj_load_init() method is triggerd TWICE before the end of the inline script is run. Looking closely, you can see that the "DomContentLoaded" event and the window "onload" event both fired prematurely.

In addtion, the the dj_load_init() method is only registered as a listener for DomContentLoaded -- not the window.onload event.

Reproducible: Sometimes

Steps to Reproduce:
We found the smallest amount of html/javascript that still demonstrates the behavior to make it easier to understand the issue. The downside is that it happens less frequently. To reproduce just load up 
http://www.vodafonegroup.eu:8080/jsp-examples/ff3/welcome.jsp
and see it alternating between this page and the return_to_welcome.jsp page. 

After a while an alert pops up saying "Could not find Clock!!!" This is when the onLoad event has fired prematurely.

Actual Results:  
You get the alert and our logging output appears on the screen

WELCOME
LOGGER RESULTS
BEGIN dojo.js.uncompressed.js
Adding dj_load_init() to DOMContentLoaded Event
END dojo.js.uncompressed.js
-------------------
BEGIN INLINE SCRIPT
-------------------
Before dojo.require(dojo.widget.Clock)
Begin dj_load_init(). (Triggered from EVENT: domcontentloaded)
dojo.hostenv.makeWidgets() Called
dojo.hostenv.makeWidgets() Ended
Begin dj_load_init(). (Triggered from EVENT: load)
After dojo.require(dojo.widget.Clock)
Before dojo.addOnLoad(init)
init() Called
EXPECTED RESULTS
-------------------------------------------------------------------------
BEGIN dojo.js.uncompressed.js
Adding dj_load_init() to DOMContentLoaded Event
END dojo.js.uncompressed.js
-------------------
BEGIN INLINE SCRIPT
------------------
Before dojo.require(dojo.widget.Clock)
After dojo.require(dojo.widget.Clock)
Before dojo.addOnLoad(init)
After dojo.addOnLoad(init)
-------------------
END INLINE SCRIPT
-------------------
Begin dj_load_init(). (Triggered from EVENT: domcontentloaded)
dojo.hostenv.makeWidgets() Called
dojo.hostenv.makeWidgets() Ended
init() Called
init() Finished

Expected Results:  
It should load the entire page, BEFORE firing onLoad

The problem seems to be most prevalent on windows. We can send you server side code on request.

This issue is causing major havoc on our site, which is heavily web2.0. And when the javascript loading breaks it leaves the site pretty much broken. We see this issue in ff3 only (tested ff2+, IE6/7, Safari).

See also forum entry:
http://forums.mozillazine.org/viewtopic.php?f=38&p=3774055
Tested on the occurrence of the error (on Windows XP), regression range is
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1184111160&maxdate=1184115659
In those days I saw a blue clock but not in the last builds anymore.
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Hi Ria,

Thank you for investigating this quickly. I just wanted to make sure you also saw the alert box popping up (and not just the clock picture not showing up). I'll attach a screenshot with the alert right after I post this. Please let us know what else we can do to help.
Attached image clock not found alert
This alert box shows up when the resource loading goes bad, which is intermittent.
Yes, this was the "error" I meant in comment 1.
Neither regression range seems to make much sense to me....

I certainly agree that DOMContentLoaded should not be firing while the inline script is executing, unless something weird like stop() being called is happening under dojo.require.

It'd be lovely to have a reproducible testcase, better yet one that didn't rely o the whole of dojo being present.
Tested it some minutes longer and I'm getting the same result with this testcase.
I was not able to reproduce the problem on all Windows computers BTW.
Hmm.  So the deal is that the inline script does a sync XMLHttpRequest load, right?  sicking, peterv do we unblock the parser before executing the inline script?  That is, if we spin the event loop in said inline script, can that restart the parser during the script execution?  That seems undesirable, if so.
Related to bug 444457, perhaps?

Fwiw, I tried to come up with something useful, I could reproduce it sometimes with the dojo version that I uploaded here (and used in combination with a local webserver):
http://martijn.martijn.googlepages.com/dojo.zip
But the bug only happened sporadically.
> Related to bug 444457, perhaps?

Sort of, in the sense that in both cases we end up spinning the event queue during a sync XHR, which we didn't use to do before threadmanager.

But this is a parser bug at heart if we're talking about DOMContentLoaded and onload firing.

That said, I set up a testcase at http://landfill.mozilla.org/ryl/XMLHttpRequest-onload.html that in fact does not seem to show the problem... so I'm not sure what _does_ trigger it.
Hi guys,

Thanks for looking into this. Some more datapoints

1. pages with more resources being loaded show the issue much more frequent. But we tried to remove as much code as we could and still see the issue. However it occurs a lot less in the example. (smells like a threading issue to me)
2. The issue is easier to reproduce if the page is a jsp, so that it cannot be cached by firefox.
3. We added some extra debug out out to our initial example, and the new example can be found at http://www.vodafonegroup.eu:8080/jsp-examples/ff3e/welcome.jsp.
4. Essentially, dojo is using an XmlHttpRequest to grab the javascript and then eval() it into the global script space. That's why we're seeing the
http.open('GET', '/dojo/src/someScript.js', false)
http.send()
(note: the http variable is an XmlHttpRequest object)
The additional debugging shows that the DomContentLoaded event is triggered while the http.send() instruction is being executed. Is there some sort of an issue around executing xmlhttprequests in the global space before the page has loaded?
5. We tried *just* making the xmlhttprequests (not using dojo), but then we were only able to reproduce it once, so the issue still seems to be there but I think ff is not doing enough to make it the issue happen reliably, see the item 1 on this list.

Hope this helps,

--Kurt
Kurt, thanks for confirming that the DOMContentLoaded fires during the sync XHR.  That's what I was guessing was happening, but I haven't been able to reproduce it with the testcase I pointed to in comment 10...

If you can reliably trap the erroneous event firing, would you be able to catch it in venkman and possibly come up with a JavaScript stack that shows whether there's some other JS running under the send() call?
Yes we think you are right on. We'll try to get the stack trace. 

You can also try our setup. You can download the entire tomcat + example at:
http://www.vodafonegroup.eu:8080/jsp-examples/ff3/tomcat.tar.gz

--Kurt

Note that "Begin dj_load_init(). (Triggered from EVENT: domcontentloaded)" fires fairly early on, and ff remains loading other JS widgets.
Attachment #329703 - Attachment mime type: text/plain → text/html
I think that I've the same problem on my JavaScript code, the onload code is called when the page is not fully loaded.

You have found a workaround ?
Not really, we upgraded to Dojo 1.1, but that didn't help. Putting all your javascript files in one download seems to make things less fragile though.
I've found a workaround for my problem.
If the call to the initialization JavaScript code is done at the end as last element of the body tag, it work fine.
Luca Palli, do you possibly have a testcase that reliably reproduces the problem?  I'm still trying to construct one so that I can debug it...
Blocks: 448625
Hi Boris,

Did you try this setup?

http://www.vodafonegroup.eu:8080/jsp-examples/ff3/tomcat.tar.gz

Thx,

--Kurt
Kurt, I haven't had a chance to get that set up yet.  It's on my list of things to try doin, but since it seems like a pretty big time investment I need to find such a chunk of time.  But in any case, that will be identical to the testcase at the URL in the URL field, right?  Just debugging that is also on my list, and it is again a very large time investment due to the complexity of the code.  Hence the quest for a simple testcase that reproduces the problem if we can find one.  We'll eventually need one anyway for the regression test, so it's just a matter of whether it's constructed based on the patch or vice versa...
Yep it is the same URL, we'worked very hard to make this test as small as possible and made it such that you can simply let it run until it hits the issue. With most threading bugs (at least that's what I think it is) things are never 'easy' to reproduce :(. However, I understand, but more than half our ff users are now converted to ff3. 
I am running in to this bug as well.  I thought I would mention that the behavior for me seems to be much more frequent when I have firebug enabled and the console showing.  I bring this up for two reasons.  If its a threading issue, then firebug might be doing something to exacerbate the issue.  Secondly, its possible that caching vs not caching might be a factor and certain firebug setting seem to affect caching as well.

Anyway, seems like this one is a hard one to nail down.  Its hitting me pretty hard at the moment.  Any updates?
It's not a "threading" issue, since everything involved runs on a single thread.  It might be an event delivery order issue...

Michael, if you can nail down a 100% (or even 90%) reproducible set of steps, with a not-too-complicated page, that would be a huge help.
My company has also run into this issue, as well as another problem which may be related.  It seems that Firefox 3, unlike its predecessor, executes Javascript in the document <body> before the <head> is finished loading.  

We have a case in which it is necessary to have a call to "dojo.addOnLoad" in the <body>, which has some dependencies on code that executes in the <head>.  In Firefox 2 (and every other browser we've tested), the <head> loads completely before the <body> is displayed and any scripts within it executed, so the addOnLoad in the <body> is safely executed AFTER all its dependencies are loaded, and after any addOnLoad calls that are made in the <head>.  

In Firefox 3, the <body> is displayed and executed as soon as it is downloaded from the server, so if there is any code in the <head> which takes some time to execute (like dojo.require() calls), it may not be finished by the time the <body> scripts are executed.  This has all sorts of implications for timing and dependencies.  

It's perhaps worthwhile to note that this behavior causes pages in FF3 to _appear_ to load much faster than in FF2, which would show a blank page while all the dojo in the <head> executed.

I am working on proprietary code but will attempt to get a URL cleared that can demonstrate the problem.
Scott, I would be very interested in your example.  What you described should not be happening!
Scott, any luck with getting an example URL made available?  I am admittedly, not that versed in debugging Firefox and have been having trouble coming up with a clean example to provide Boris. 
Sorry guys, we're in the middle of app testing right now so it's been hard to look into this any further.  For the time being we just had to work around the problem by moving all of of the "dojo.addOnLoad" statements in the <BODY> into a function that repeatedly checks for successful completion of all the code in the <HEAD>.  When I get some breathing room I'll try to make a stripped down page that demonstrates the problem.  
Ok, we have posted a page reproducing the error I described in comment #24.  It can be viewed at http://gchris.googlepages.com/444322.html.  You can also download a zip with the three files you need to reproduce the issue locally at http://gchris.googlepages.com/444322.zip.

We were able to reproduce this locally 100% of the time.  On the remote server the bug may not occur if the page is cached, but you can see it again by putting a unique query string at the end of the URL.

In a nutshell, the issue occurs when you include an external Javascript file AND  make a synchronous AJAX call in the <head> of a document -- but the document file size has to be relatively large.  A full description is included on the example page.

I hope this helps pinpoint the problem!
Scott, thank you!  I can indeed reproduce reliably with that testcase, and I think I see what's happening here.  I need to do a bit more digging through the code, but I hope to have a pretty complete analysis of what's going wrong done sometime tomorrow morning and then see what we can do to fix this.
OK.  So here's the story.  Comment 8 was sort of on top of things; I just couldn't find the mechanism that caused the lossage.

The parser can be either enabled or disabled.  Switching this state just affects what it will do the next time it's told to parse.  If it's disabled, it will ignore that, and if not it will parse.

There are two ways (for purposes of this bug) that a parser can be told to parse.  The first is that the content sink tells it to do so.  The second is that more data for the parser arrives from the network.

The current code flow when parsing a <script> tag (or rather the </script> tag) is as follows:

1)  Insert the node into the DOM.
2)  If inline script, run the script.  If not, start the script load.
3)  If we started a script load, block the parser.

Now if we had an inline script we just go on with life.  Otherwise:

4)  When the script loads, unblock the parser.
5)  Run the script
6)  If the parser is now not blocked, post an event to tell the parser to
    parse.

Data arriving from the network also posts events to tell the parser to parse, effectively.  Now it used to be that sync XHR would not process any of those events, but now it does.  So here's what happens in Scott's testcase:

* We start the external script load and block the parser
* We run the external script, unblock the parser, and post an event telling it
  to parse.
* We get a notification that more data is available from the network and start
  parsing.
* We parse the inline script and start the sync XHR
* We get the event posted by the external script and start parsing again while
  the sync XHR is still in progress.

After this, we lose, of course.  Note that this sequence of events depends on the "more data from the network" notification coming after we finished loading the extenal script, which is why the page size matters and the cache state matters.  Also, the external script is key, because we won't get another "data from the network" notification while we're in the middle of one already.

I think we mostly have two options here.  Option 1 (the "correct" fix, in some sense) is blocking the parser before appending the script, then unblocking it if it turns out the script was inline.  Then when we go to run the script, not unblocking the parser till after the script runs.  Option 2 is that any time the parser gets an OnDataAvailable we cancel any pending parser continue event that the sink has put up.  Or some such hack attempting to ensure that we can't have two parser continues in flight as in this testcase and hence can't get into this situation.

Option 2 is uglier, I suspect, but might be more branch-safe in terms of behavior changes.  On the other hand, I thought sicking had written pretty good script loading + parser tests at some point, and option 1 seems more robust to me.

Jonas, Blake, Peter, thoughts?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9.1?
Flags: blocking1.9.0.3?
Keywords: regression
Summary: Firefox 3 onLoad event fireing before the page is fully loaded. → Firefox 3 onload and DOMContentLoaded event firing before the page is fully loaded.
We at Adobe are also experiencing sever problems in Firefox 3 with several of our services due to this bug; in fact, it's affecting our customer to such an extent that we're willing to hire someone specifically to make a patch to fix this bug (at least enough that our software works).

I apologize if this is not the right venue to make this offer -- I've been searching the mozilla sites for some central "hiring office" -- but I also think those most familiar with this issue and most likely to have the solution in any case would be this bug's CC list.

Please email me if you're interested (and again, I apologize in advance if this is not the right venue, please direct me to the right place).
For what it's worth, option 2 from comment 30 doesn't really work.  Canceling the parser continue event doesn't help the situation where the parser gets continued off the parser continue event, we start running a script, and then while we're doing that an OnDataAvailable comes in.  So we really need to do option 1 here.
This is a regression as it works properly using FF2. 

So I wonder if it's worth reviewing the deltas of the code in question to see if something can possibly be reverted or at least better grok what exactly broke, where, and why.
Pete, please read the bug.  At this point we know exactly what's going on here; all that's needed is a good bit of changing of fragile code to fix it.
Unfortunately we can't simply delay unblocking the parser since then document.writes inside the script wouldn't be acted on immediately :(

Ideally what we should have is a way to block incoming network traffic, while still allowing document.write to work. I.e. some sort of semi-blocked state.
Blake and I talked about this yesterday; he suggested that any time we're between starting script execution and firing the script's continue event we simply don't ResumeParse from OnDataAvailable.  We think this should fix the problem and be reasonably safe; he was going to take a stab at it in the next few days.
Attached patch patch v1 (obsolete) — Splinter Review
This appears to fix the bug. Getting the calls to ScriptExecuting and ScriptDidExecute to fire in all of the right places was a real pain.

I noticed that nsContentSink::ContinueInterruptedParsing was unused so I removed it.

I also noticed that if an asynchronous script document.write()s another asynchronous script, we never remove the first script from nsContentSink::mScriptElements. I don't think that there are any ill effects of this, but we might want to fix it at some point.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #340080 - Flags: review?(bzbarsky)
Component: General → Content
QA Contact: general
We just drop the sink once we finish parsing, so that might not be a big deal.  reading patch now!
Comment on attachment 340080 [details] [diff] [review]
patch v1

>+++ b/content/base/src/nsContentSink.cpp
>@@ -340,6 +340,9 @@ nsContentSink::ScriptAvailable(nsresult 
>                                nsIURI *aURI,
>                                PRInt32 aLineNo)
> {
>+  // Do this no matter what.
>+  mParser->ScriptExecuting();

That's not right, is it?  It's perfectly possible to get a ScriptAvailable() without ever getting a ScriptEvaluated(), and in that case you'll end up never calling ScriptDidExecute(), no?  In particular, you should probably only call this if either aResult is success (indicating that we'll execute the script) or we actually post the event down in the "failure, script we were waiting for, not binding aborted" branch.

Should be easy to add a test for this (e.g. a script that's 404), I would think.

Other than that, looks good.  I wonder whether we can sanely add a regression test for this bug itself...
(In reply to comment #39)
> That's not right, is it?

Oops, yeah. I got tunnel vision.

I was going to try to see if I could make a mochitest out of Scott's testcase.
Attached patch patch v2 (obsolete) — Splinter Review
Note that this patch is truncated because the added test is *long* in order to trigger the bug.

In nsContentSink::ScriptAvailable, calling ScriptExecuting is OK because we'll call ContinueInterruptedParsingAsync later on (guaranteed by the added assertion) which calls ScriptDidExecute.
Attachment #340080 - Attachment is obsolete: true
Attachment #340080 - Flags: review?(bzbarsky)
Attachment #340182 - Flags: review?(bzbarsky)
Comment on attachment 340182 [details] [diff] [review]
patch v2

Huh, I thought I asked for review on this when I attached it.
Comment on attachment 340182 [details] [diff] [review]
patch v2

>+++ b/content/base/src/nsContentSink.cpp
>+    NS_ASSERTION(mParser->IsParserEnabled() ||
>+                 (count > 0 && aElement == mScriptElements[count - 1]),
>+                 "Possibly calling 
ScriptExecuting without ScriptDidExecute");

I don't think that assert is correct (but bear with me).  In particular, what happens in this case:

<script>
  var s = document.createElement("script");
  s.textContent = "alert('testing')";
  document.documentElement.appendChild(s);
</script>

I guess we get ScriptAvailable for the parsed <script> and enable the parser, so that by the time we get ScriptAvailable for the appended script we're already enabled... but then we'll call ScriptExecuting for the appended script, and then ScriptDidExecute.  And the parser will start listening for OnDataAvailable, all while the parsed script is still executing?

I guess that's a problem with document.write of a <script> too; it sounds like we really need to keep track of a "number of scripts currently executing" counter or something.
Comment on attachment 340182 [details] [diff] [review]
patch v2

r- per discussion; need to be more careful about dealing with DOM-created scripts.
Attachment #340182 - Flags: review?(bzbarsky) → review-
QA Contact: content
Here is an extremely simple workaround that worked for me. The nice thing about it is that it does not make you alter or move your existing code. Just insert the following before the closing </body> tag:

<script type='text/javascript'> </script>

NOTICE: there is a space between the "script" tags, as in order for this workaround to be effective, there must be something between the script tags - totally empty tags will not workaround the issue.

Hope it helps.
Blocking 1.9.0.4 assuming it's going to get blocking 1.9.1 (see comment 31 for real-world impact), but if it's not taken for trunk then we're not going to take unwanted changes to fragile code on the branch.
Flags: blocking1.9.0.4? → blocking1.9.0.4+
Guess it didn't... we'll take it on the branch when the trunk gets fixed.
Flags: blocking1.9.0.4+ → wanted1.9.0.x+
It seems disabling firebug greatly decreases the occurrence of this issue.  I was nearly sure it solved it, but after a few minutes of testing it still randomly occurs.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
I'm afraid the fix from comment 45 didn't work consistently in our original test case (http://gchris.googlepages.com/444322.html).  It does seem to work when the page is cached, but not on first load.
Attached patch patch v3Splinter Review
This should do it. The parser will now ignore an OnDataAvailable when any script is executing.
Attachment #340182 - Attachment is obsolete: true
Attachment #350893 - Flags: superreview?(bzbarsky)
Attachment #350893 - Flags: review?(bzbarsky)
Comment on attachment 350893 [details] [diff] [review]
patch v3

Looks good.  Make sure we have a test covering comment 43?
Attachment #350893 - Flags: superreview?(bzbarsky)
Attachment #350893 - Flags: superreview+
Attachment #350893 - Flags: review?(bzbarsky)
Attachment #350893 - Flags: review+
From talking to mrbkap and looking at the patch (and ignoring the GIANT test), this looks pretty straight forward to backport to Firefox 3. The one difference will probably be that we'll need an nsIParser2 interface to avoid changing the existing one, but that's pretty straight forward...
Checked into mozilla-central: http://hg.mozilla.org/mozilla-central/rev/f669ee7e9871
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Great news, thanks for all the hard work guys. Will there be a nightly build tomorrow that we can download to test?

Thx again,

--Kurt
Yes, here: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/

Please test the new build tomorrow, Kurt. And thanks for all the help that made it possible to fix this bug.
Let's make sure we get this in 1.9.0 branch too.
Flags: in-testsuite+
Flags: blocking1.9.0.6?
It indeed seems to be fixed for me, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081212 Minefield/3.2a1pre
I tested with the url for 20 minutes or so.
Blocking for 1.9.0.6, but we'll need the interface changes moved into a nsIParser_MOZILLA_1_9_0_BRANCH that inherits from nsIParser to avoid interface incompatibilities.
Flags: blocking1.9.0.6? → blocking1.9.0.6+
Whiteboard: Needs 1.9.0 branch patch
Whiteboard: Needs 1.9.0 branch patch → Needs 1.9.0 branch patch, needs 1.9.1 landing
Did this make it into FF 3.0.5?  Or does 1.9.0.6 correspond to FF 3.0.6?
(In reply to comment #59)
> Did this make it into FF 3.0.5?  Or does 1.9.0.6 correspond to FF 3.0.6?

1.9.0.6 refers to the core that will ship with 3.0.6, yes.
Attached patch Diff for the 1.9.0 branch (obsolete) — Splinter Review
This is the diff between the trunk patch and the branch patch.
Attachment #355653 - Flags: superreview?(jst)
Attachment #355653 - Flags: review?(jst)
Attachment #355653 - Flags: approval1.9.0.6?
I also had to sprinkle some do_QueryInterface magic in ns{XML,HTML}ContentSink.
Attachment #355653 - Flags: superreview?(jst)
Attachment #355653 - Flags: superreview+
Attachment #355653 - Flags: review?(jst)
Attachment #355653 - Flags: review+
Whiteboard: Needs 1.9.0 branch patch, needs 1.9.1 landing → [needs 1.9.1 landing]
Attachment #355653 - Flags: superreview+ → superreview?(jst)
Comment on attachment 355653 [details] [diff] [review]
Diff for the 1.9.0 branch

Isn't the point of _1_9_BRANCH uglification interfaces to not change the interface or IID of the original version? Requesting re-superreview for clarification.

>-// 3007e9c0-4d3e-4c80-8cae-fbb4723d88f2
>+// {506527cc-d832-420b-ba3a-80c05aa105f4}
> #define NS_IPARSER_IID \
>-{ 0x3007e9c0, 0x4d3e, 0x4c80, \
>-  { 0x8c, 0xae, 0xfb, 0xb4, 0x72, 0x3d, 0x88, 0xf2 } }
>+{ 0x506527cc, 0xd832, 0x420b, \
>+  { 0xba, 0x3a, 0x80, 0xc0, 0x5a, 0xa1, 0x05, 0xf4 } }
>+
>+// 5FA66227-44CF-4572-9B5F-E9A357B67ED9
>+#define NS_IPARSER_1_9_0_BRANCH \
>+{ 0x5FA66227, 0x44CF, 0x4572, \
>+  { 0x9B, 0x5F, 0xE9, 0xA3, 0x57, 0xB6, 0x7E, 0xD9 } }
Yeah, we shouldn't be changing NS_IPARSER_IID.
Just as a note: the previous patch was an interdiff between the patch that landed on trunk and the patch I made for the 1.9.0 branch. The change to nsIParser's DTD was changing it back to its original value. However, it turns out that I'd accidentally ported it to the 1.9.1 branch anyway, so here's a diff against the 1.9.0 branch for real.
Attachment #355653 - Attachment is obsolete: true
Attachment #355923 - Flags: superreview?
Attachment #355923 - Flags: review+
Attachment #355923 - Flags: approval1.9.0.6?
Attachment #355653 - Flags: superreview?(jst)
Attachment #355653 - Flags: approval1.9.0.6?
Attachment #355923 - Flags: superreview? → superreview?(dveditz)
Comment on attachment 355923 [details] [diff] [review]
Updated 1.9.0 patch

Looks good

Approved for 1.9.0.6, a=dveditz for release-drivers.
Attachment #355923 - Flags: superreview?(dveditz)
Attachment #355923 - Flags: superreview+
Attachment #355923 - Flags: approval1.9.0.6?
Attachment #355923 - Flags: approval1.9.0.6+
Fix checked into the 1.9.0 branch.
Keywords: fixed1.9.0.6
Verified for 1.9.0.6 using http://gchris.googlepages.com/444322.html with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.6pre) Gecko/2009011304 GranParadiso/3.0.6pre.
I am sorry to report, I still get this in firefox 3.0.6 on fedora 10 64 bit. If I refresh:

http://gchris.googlepages.com/444322.html

multiple times out of no where it will say "Expected value undefined". I do have caching disabled on the firefox web dev toolbar, and I saw this bombing my dojo page similiarly. All of a sudden when developing my dojo dies. This does seem to happen less, but the only resolution is to close down and restart FF.
Pete, it will probably be better if you file a new bug for the problem you're seeing rather than re-opening this one.
I'm still seeing this problem too:

Cut/Paste from the defect page Pete refers to (Note the undefined) as loaded by 3.0.6 for me:
---------------------------------------------------

This page demonstrates the bug described in Bugzilla #444322.

In the <head> section of the document, the global var x is set to "scott". In the <body>, a Javascript statement writes the value of x to the screen.

The expected output is: scott.

The actual output is: undefined

This shows that the Javascript in the <head> is not completing before the script in the <body> is executed.

For this bug to occur, the following circumstances must be met:

   1. An external Javascript file must be loaded on the page. It doesn't matter what code is in the file, and it can in fact be blank as test.js is in this example.
   2. A synchronous AJAX call must be made in the <head>. Again, the file being loaded can be empty. An alert() call will also work.
   3. The size of the .html (or whatever) file must be relatively large -- in our tests anything above 169,609 bytes did the trick (hence the large amount of text following this explanation). 

On some occasions we saw the issue not occur if the page was not cached or if it was refreshed, but it would then occur if the page was hit again from a link or by pressing enter in the location bar. This example seems to cause the bug whether the page is cached or not.
(In reply to comment #71)
> Pete, it will probably be better if you file a new bug for the problem you're
> seeing rather than re-opening this one.

I am not sure why that would help? It is this bug reopened IMHO?
Because this bug already has a testcase which was fixed by the patch included here and the general policy around here is one issue per bug. It would be better to start with a clean slate as the cause may be something entirely different than this bug.
In a clean 3.0.6 I was able to reproduce the above bug once (but never again). In a Firefox 3.0.6 with Firebug 1.3.2 installed I can reproduce it 100% of the time. Perhaps this is a Firebug issue now?
Depends on: 483818
This seems to cause leak bug 483818.
Component: Content → DOM
QA Contact: content → general
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.