608669 - "chrome-document-global-created" broken since 2010-09-04

Status: RESOLVED FIXED

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

Description

[Jeferson Hultmann]

chrome-document-global-created works here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/09/2010-09-03-04-mozilla-central/

But not here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/09/2010-09-04-04-mozilla-central/

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4b879b793eb6&tochange=bd474ff6f86c


To reproduce this bug, paste the following code into Error Console:


var Cc = Components.classes;
var Ci = Components.interfaces;
Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
var obs = {
  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),
  observe: function(subject, topic, data) {
    Cc["@mozilla.org/consoleservice;1"]
      .getService(Ci.nsIConsoleService)
      .logStringMessage(topic + " " + subject.document.location.href);
  }
};
Cc["@mozilla.org/observer-service;1"]
  .getService(Ci.nsIObserverService)
  .addObserver(obs, "chrome-document-global-created", false);



Open: Tools > Downloads

2010-09-03 build - Error Console displays:
chrome-document-global-created chrome://mozapps/content/downloads/downloads.xul

2010-09-04 build - Error Console displays nothing.

Comment 1

[Alice0775 White]

Regression window:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1730f1358f13&tochange=83e9c68be99d

Comment 2

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

Is this notification something we need for FF4? It's probably easy enough to fix this, but in case it isn't, does this need to block?

Comment 3

[Jeferson Hultmann]

From my point of view (extension author), chrome-document-global-created is the best way to mimic overlays (in bootstrapped extensions).

My extension works fine without overlays in Firefox 3.6.12, but not in 4.0b8pre.

Comment 4

[Benjamin Smedberg]

This is pretty important for extension developers who are trying to use bootstrapped addons, so I think it needs to block. I'm almost saying it should block b7, but certainly b8.

Comment 5

[Benjamin Smedberg]

Jeferson, would you like to create a chrome-mochitest for this, perhaps?

Comment 6

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

Attachment: Patch to fix

So the notification partially works. For an <iframe> things works as expected. However when calling window.open things don't work as well.

The problem is that we first call nsGlobalWindow::SetNewDocument with a non-chrome-about-blank document. This cause the code to send out a
content-document-global-created notification. When we then later insert a chrome document, we (correctly) detect that the inner window is being reused and don't send out a new notification.

This patch makes use use a has-notificed flag to determine if we should notify, rather than checking if we're reusing the inner window. It then avoids notifying if we're loading a non-chrome window into a chrome-docshell.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 492479 [details] [diff] [review]
Patch to fix

- In nsGlobalWindow::nsGlobalWindow():

     mCleanedUp(PR_FALSE),
     mCallCleanUpAfterModalDialogCloses(PR_FALSE),
     mDialogAbuseCount(0),
     mDialogDisabled(PR_FALSE)
 {
+  mHasNotifiedGlobalCreated = PR_FALSE;

Move this up...

r=jst

Comment 8

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

Can't do that. You can only initialize things in the colon-list which live in the class whose ctor is running... gcc was kind enough to remind me of this.

So the only way to initialize this in a colon list would be to give nsPIGlobalWindow a ctor which doesn't seem worth it.

Comment 9

[Johnny Stenback (:jst)]

In that case, initialize it in the nsPIDOMWindow ctor, also in nsGlobalWindow.cpp.

Comment 10

[Johnny Stenback (:jst)]

Oh, and yes, it does already exist, it's just not where you'd think it'd be :)

Comment 11

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

Checked in: http://hg.mozilla.org/mozilla-central/rev/4b9ba5049e66

Please do test that it's working as expected now. It seemed like it was mostly working, so I've only fixed the one broken case that I found.

Comment 12

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)

Comment 13

[Henri Sivonen (:hsivonen) (on leave)]

The comments that got landed says:
    4.35 +    // We should probably notify. However if this is the, arguably bad,
    4.36 +    // situation when we're creating a temporary non-chrome-about-blank
    4.37 +    // document in a chrome docshell, don't notify just yet. Instead wait
    4.38 +    // until we have a real chrome doc.

With my current patch for bug 543435, the mochitest added for this bug fails, because it gets one more notification than it expects--presumably for the initial about:blank.

Should I edit the test or make a special effort to suppress notifications for the initial about:blank as the comment above suggests? I don't see how the code below the comment does what the comment says. Why is 
    4.45 +    if (itemType != nsIDocShellTreeItem::typeChrome ||
    4.46 +        nsContentUtils::IsSystemPrincipal(mDoc->NodePrincipal())) {
expected to exclude the initial about:blank?

Comment 14

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

Comment on attachment 492479 [details] [diff] [review]
Patch to fix

>+    if (itemType != nsIDocShellTreeItem::typeChrome ||
>+        nsContentUtils::IsSystemPrincipal(mDoc->NodePrincipal())) {
>+      newInnerWindow->mHasNotifiedGlobalCreated = PR_TRUE;
>+      nsContentUtils::AddScriptRunner(
>+        NS_NewRunnableMethod(this, &nsGlobalWindow::DispatchDOMWindowCreated));
>+    }
Why this?
Dispatch the event when docshell isn't chrome or document is chrome?

Comment 15

[(no longer active)]

Over in bug 795015, I have rediscovered comment 13 and 14.  The code there does not do what it advertizes, which causes us to receive chrome-document-global-created on about:blank documents.

Comment 16

[(no longer active)]

And FWIW this patch seems to fix the issue, but then the test added here fails:

diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
@@ -2140,18 +2140,23 @@ nsGlobalWindow::SetNewDocument(nsIDocume
     // document in a chrome docshell, don't notify just yet. Instead wait
     // until we have a real chrome doc.
     nsCOMPtr<nsIDocShellTreeItem> treeItem(do_QueryInterface(mDocShell));
     int32_t itemType = nsIDocShellTreeItem::typeContent;
     if (treeItem) {
       treeItem->GetItemType(&itemType);
     }
 
-    if (itemType != nsIDocShellTreeItem::typeChrome ||
-        nsContentUtils::IsSystemPrincipal(mDoc->NodePrincipal())) {
+    bool isAboutBlank = true;
+    nsIURI* docURI = aDocument->GetDocumentURI();
+    if (docURI) {
+      isAboutBlank = NS_IsAboutBlank(docURI);
+    }
+
+    if (itemType == nsIDocShellTreeItem::typeChrome && !isAboutBlank) {
       newInnerWindow->mHasNotifiedGlobalCreated = true;
       nsContentUtils::AddScriptRunner(
         NS_NewRunnableMethod(this, &nsGlobalWindow::DispatchDOMWindowCreated));
     }
   }
 
   return NS_OK;
 }

Comment 17

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

That patch certainly looks correct to me, but I have completely swapped out this stuff from my head. Please do file a new bug and attach a patch there.

Comment 18

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

Doesn't look right to me.
If one explicitly loads about:blank to a chrome docshell, we should notify about it.

Comment 19

[Henri Sivonen (:hsivonen) (on leave)]

(In reply to Olli Pettay [:smaug] from comment #18)
> Doesn't look right to me.
> If one explicitly loads about:blank to a chrome docshell, we should notify
> about it.

about:blank in chrome docshells that hold Web content is a bit of a disaster. Look for INTERNAL_LOAD_FLAGS_FIRST_LOAD in the current WIP patch for my permanemesis bug 543435. The code sharing between XUL <browser> and HTML <iframe> makes it really hard to deal with this in a reasonable way. Some docshells start by loading about:blank once, some twice (once for initialization in the XUL land and a second time in the Web land). You cannot rely on seeing “first load” flags consistently. From within the docshell, you don’t know which situation you are in (except for the bookkeeping in my patch).

Good luck and my condolences to anyone who attempts to make it less broken.

Comment 20

[(no longer active)]

I filed bug 795961 about this.  It would be good if people who understand what's going on here would comment there.  Thanks!