Closed Bug 608669 Opened 9 years ago Closed 9 years ago

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

Categories

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

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: hultmann, Assigned: sicking)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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.
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?
Blocks: 582176
blocking2.0: --- → ?
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.
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.
blocking2.0: ? → beta8+
Jeferson, would you like to create a chrome-mochitest for this, perhaps?
Assignee: nobody → jonas
blocking2.0: beta8+ → beta9+
Attached patch Patch to fixSplinter Review
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.
Attachment #492479 - Flags: review?(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
Attachment #492479 - Flags: review?(jst) → review+
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.
In that case, initialize it in the nsPIDOMWindow ctor, also in nsGlobalWindow.cpp.
Oh, and yes, it does already exist, it's just not where you'd think it'd be :)
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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)
No longer blocks: 549539, 582176
blocking2.0: beta9+ → betaN+
Blocks: 549539, 582176
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 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?
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.
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;
 }
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.
Doesn't look right to me.
If one explicitly loads about:blank to a chrome docshell, we should notify about it.
(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.
Depends on: 795961
I filed bug 795961 about this.  It would be good if people who understand what's going on here would comment there.  Thanks!
No longer depends on: 795961
Depends on: 795961
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.