567357 - Fire a DOMWindowCreated DOM event to match the observer notification in bug 549539

Status: RESOLVED FIXED

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

Description

[Benjamin Smedberg]

Bug 549539 added a (chrome|content)-document-global-created observer-service notification. People using the message manager, however, don't have easy access to the observer service and really want to be scoped to the current tab and its descendent frames anyway. This patch adds a chrome-listener-only DOMWindowCreated event which has the same effect (fires before any content script is run).

There is one issue I'm not sure about here: this patch ends up issuing a WARNING: fix the caller! file c:/builds/mozilla-central/ff-debug/content/events/src/../../../../src/content/events/src/nsEventDispatcher.cpp, line 512

because DocumentViewerImpl::InitInternal holds an nsAutoScriptBlocker. This means at least that any observers of (chrome|content)-document-global-created, as well as any event listeners of this new DOM event, need to be careful not to cause re-entrancy into DocumentViewerImpl::InitPresentationStuff. The script-blocker was added in bug 528493. Is there some way to silence this warning by refactoring InitInternal so that it actually is safe to run any scripts during that time, suppressing InitPresentationStuff?

Comment 1

[Benjamin Smedberg]

Attachment: DOMWindowCreated, rev. 1

Comment 2

[Boris Zbarsky [:bzbarsky]]

Don't we need something similar for the window-destroyed case, then?  Except there's no obvious place to dispatch the event for that one....

Comment 3

[Benjamin Smedberg]

None of my current candidate-consumers do: they typically just add something to the global object like window.Mochitest = mychromething, or window.InstallTrigger, or add an event listener to the document for things like form autofill, etc.  None of those clients really care about when the document goes away.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, ok.

Will chrome consumers in e10s receive observer service notifications at all?  Or do we need a different setup for the consumers who do care about window destruction?  Note that the destruction message is async, so can be remoted easily as needed.

Comment 5

[Benjamin Smedberg]

Not sure what "chome consumers" you mean. The observer service will function normally in both the chrome and content processes. It's just that my current plan is that the content process will only run builtin XPCOM components (libxul, plus maybe platform JS components), and extensions/apps won't have the opportunity to register contractids/category entries in the content process.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Not sure what "chome consumers" you mean.

Firebug, for one.  It wants to know when it can forget all the script info it cached for a window.

Comment 7

[Benjamin Smedberg]

I don't think we know how Firebug is going to work yet in content-land.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 446714 [details] [diff] [review]
DOMWindowCreated, rev. 1

+  if (domEventDoc)
+    domEventDoc->CreateEvent(NS_LITERAL_STRING("event"), getter_AddRefs(event));

{}

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Please don't add DOM events which fire at unsafe time.

But is the script blocker still needed in DocumentViewer?
nsSubDocumentFrame::ShowViewer is called async nowadays.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Are we talking the blocker in InitInternal?  I don't see how async would help with the flushing problem that blocker tries to address...

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I was just looking at the stack trace for which the blocker was added, but
perhaps I misinterpreted that.

Comment 12

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

I agree with comment 9. Could you add a scriptrunner (nsContentUtils::AddScriptRunner) which fires the event?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Will that still guarantee that the event fires before content script can run?

Comment 14

[Boris Zbarsky [:bzbarsky]]

I guess it should, more or less by definition....

Comment 15

[Benjamin Smedberg]

Attachment: DOMWindowCreated, rev. 2

Scriptrunner FTW

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

would this patch solve this assertion:
###!!! ASSERTION: Want to fire mutation events, but it's not safe: '(aNode->IsNodeOfType(nsINode::eCONTENT) && static_cast<nsIContent*>(aNode)-> IsInNativeAnonymousSubtree()) || sScriptBlockerCount == sRemovableScriptBlockerCount', file /home/joel/mozilla/src/content/base/src/nsContentUtils.cpp, line 3419

Comment 17

[Benjamin Smedberg]

That depends on the stack at the time.

Comment 18

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

Comment on attachment 447911 [details] [diff] [review]
DOMWindowCreated, rev. 2

Use nsContentUtils::DispatchChromeEvent instead, that'll save you much of this code.

r=me with that

Comment 19

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/be68a65eb4c7

Comment 20

[Dietrich Ayala (:dietrich)]

could this have caused the 100ms startup regression in bug 574457?

Comment 21

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

Fixed on 1.9.2 as part of bug 549539

Comment 22

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

(In reply to comment #20)
> could this have caused the 100ms startup regression in bug 574457?

That seems very unlikely. Simply firing the notification when no one is listening should be a very low cost operation. Especially in the context of creating a new global object.