Closed Bug 642145 Opened 9 years ago Closed 9 years ago

'document-element-inserted' event is fired when inserting non-element nodes to the document node

Categories

(Core :: DOM: HTML Parser, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5
Tracking Status
blocking2.0 --- -
status2.0 --- .1-fixed

People

(Reporter: ochameau, Assigned: hsivonen)

References

Details

Attachments

(2 files, 1 obsolete file)

Here at Jetpack project we discover a bug around 'document-element-inserted' event fired throught nsIObserverService.

Here is a testcase that can be run directly in JS Console:

#########################################

var Cc = Components.classes; 
var Ci = Components.interfaces;

var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].getService(Ci.nsIWindowWatcher);
var win = ww.openWindow(null, "data:text/html,<!DOCTYPE%20html><h1>hello</h1>", "test", "chrome,centerscreen", null);

var obs = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
obs.addObserver({
    observe: function (subject, topic, data) {
      Components.utils.reportError("Event : "+subject.location.href);
    }
  }, 'document-element-inserted', false);

#########################################

You will see two errors messages whereas we are waiting for only one.
And if you remove the "<!DOCTYPE%20html>" from the call to openWindow, and so load a non-HTML5 web page, you are going to receive only one event.


Here is the supposed stack trace:

nsHtml5TreeOperation::Perform
  http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.cpp#339

nsHtml5TreeOperation::AppendToDocument
  http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.cpp#250

nsContentUtils::AddScriptRunner
  http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#4738

nsDocElementCreatedNotificationRunner::Run
  http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.cpp#240

nsContentSink::NotifyDocElementCreated
  http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentSink.cpp#1665
I assume this would be the case for any standards doctype, right?  We don't have any "non-HTML5 web pages".

Henri, why would this differ between quirks and standards mode?
Component: General → HTML: Parser
QA Contact: general → parser
Summary: 'document-element-inserted' event is fired twice on HTML5 documents → 'document-element-inserted' event is fired twice on strict-mode documents
Is this a difference between quirks and standards or a difference between no doctype and any doctype?

What happens with <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2 Final//EN"> ?
(In reply to comment #2)
> Is this a difference between quirks and standards or a difference between no
> doctype and any doctype?
> 
> What happens with <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2 Final//EN"> ?

It is the same than <!DOCTYPE html>, you have two events fired.
Component: HTML: Parser → General
Component: General → HTML: Parser
OK. The problem is simply that http://hg.mozilla.org/mozilla-central/rev/34afbe32fb0e made the notification fire when *any* node is appended to the document. In addition to the root element, a doctype or comments may be appended to the document.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Summary: 'document-element-inserted' event is fired twice on strict-mode documents → 'document-element-inserted' event is fired when inserting non-element nodes to the document node
Attached patch Test with universalXPconnect (obsolete) — Splinter Review
What changes should I make to remove universalXPConnect? I tried to make this a chrome mochitest, but I failed to find the right boilerplate for the harness inclusion.
Attachment #520168 - Flags: feedback?(jonas)
You can get chrome mochitest boilerplate by running:

  testing/mochitest/gen_template.pl -type chrome -b 642145

Or were you looking for something else there?
(In reply to comment #7)
> You can get chrome mochitest boilerplate by running:
> 
>   testing/mochitest/gen_template.pl -type chrome -b 642145
> 
> Or were you looking for something else there?

I was looking for an HTML template. Do chrome-privileged mochitests have to be bootstrapped from XUL? (I already tried using the scripts from the XUL template, but they didn't work right in HTML.)
> Do chrome-privileged mochitests have to be bootstrapped from XUL?

Not sure, but it wouldn't surprise me. :(
Depends on: 579764
We should get this into Macaw so this new Firefox 4 feature works the way folks expect it to on the many DOCTYPE-wielding pages out there.
blocking2.0: --- → ?
Attached patch Chrome mochitestSplinter Review
Attachment #520168 - Attachment is obsolete: true
Attachment #520168 - Flags: feedback?(jonas)
Attachment #522624 - Flags: review?(jonas)
Hmm. Looks like the test case triggers bug 635548. :-( Need to experiment more with the test case. The fix itself is ready, though.
I suggest landing the fix separately from the test so that the fix can make it to Firefox 5 even if the test has to wait for bug 635548 to get fixed.
(In reply to comment #10)
> We should get this into Macaw so this new Firefox 4 feature works the way folks
> expect it to on the many DOCTYPE-wielding pages out there.

What's the status of Macaw? Does the fix for this patch have any chance of making it anymore? The deadline for Firefox 5 is also near.
Fix pushed to cedar:
http://hg.mozilla.org/projects/cedar/rev/119dd70688e2

Test *not* pushed per comment 13.
Flags: in-testsuite?
Whiteboard: [fixed-in-cedar]
This can't block macaw at this point, but you still have a couple of days to land on trunk and request mozilla2.0 approval to get it in.
blocking2.0: ? → -
http://hg.mozilla.org/mozilla-central/rev/119dd70688e2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla2.2
Comment on attachment 520169 [details] [diff] [review]
Check for elementness before firing the notification

Requesting approval for 2.0 in reference to comment 10, comment 16 and comment 17.
Attachment #520169 - Flags: approval2.0?
Comment on attachment 520169 [details] [diff] [review]
Check for elementness before firing the notification

Approved for Macaw but we won't block on it.
Attachment #520169 - Flags: approval2.0? → approval2.0+
Marking as dependent of bug 635548, since that bug is blocking the landing of the test case here.
Depends on: 635548
Target Milestone: mozilla5 → ---
Version: Trunk → Other Branch
Shouldn't this be marked as "status2.0: .1-fixed"?
Target Milestone: --- → mozilla5
You need to log in before you can comment on or make changes to this bug.