Closed Bug 330458 Opened 18 years ago Closed 6 years ago

Cannot dynamically load an overlay using document.loadOverlay until a previous overlay is completely loaded

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: glazou, Unassigned)

References

(
URL
)

Details

(Keywords: assertion, testcase)

Attachments

(2 files, 2 obsolete files)

Unbitrotted WIP patch v0.3
38.26 KB, patch
Details | Diff | Splinter Review
different testcase, v1
4.88 KB, application/zip
Details 
Tested on Firefox 1.5.0.1 on WinXP.

Two consecutive |document.loadOverlay()| in JS are totally failing. Apparently, we need to wait for completion of the first one to call the second one. That's a design flaw and we should have a queue here.
This bug is painful because you have to rely on observers and write a queue manager yourself...

See attached test case: http://glazman.org/overlays/overlays.html
From a duplicate:

Another testcase: http://mozilla.doslash.org/stuff/overlays-test/test9.xul

Additionally, the error console throws many exceptions (this from the trunk):
Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMXULDocument.loadOverlay]"  nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://global/content/bindings/preferences.xml :: showPane :: line 647" 
data: no]

Also asserts for me:
###!!! ASSERTION: forward references have already been resolved: 'Error', file
c:/mozilla/content/xul/document/src/nsXULDocument.cpp, line 1049
and leaks on shutdown.
Keywords: assertion, testcase
OS: Windows XP → All
Hardware: PC → All
Summary: Cannot dynamically load an overlay until a previous overlay is completely loaded → Cannot dynamically load an overlay using document.loadOverlay until a previous overlay is completely loaded
Version: 1.8 Branch → Trunk
*** Bug 355038 has been marked as a duplicate of this bug. ***
I've just run into this bug.  It's serious.

A separate extension was making calls to loadOverlay, causing the call in my extension to fail.  It just happened to squeeze in first.  Thus, sections of functionality were just plain missing.

For what it's worth, still in 2.0:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Attached patch WIP (obsolete) — — Splinter Review 
Need to make sure the load observers are processed correctly with this change and address the XXXs in the diff. Also need to convert my testcases to proper reftests (use window.location to work around inability to pass relative URLs to document.loadOverlay() - bug 315988).
Assignee: nobody → asqueella
Status: NEW → ASSIGNED
Depends on: 282103
A comment in nsXULDocument.cpp claims we don't support loading the same overlay twice into the same document, because it "doesn't make sense anyway":
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/xul/document/src/nsXULDocument.cpp&rev=1.752&mark=2658-2659#2658

In fact we do support it, both with document.loadOverlay (provided that there's a sufficient interval between the two loadOverlay calls) and normal overlays, as far as I can see. I don't see why we should forbid that.

So how should this work?
Attached patch WIP #2 (obsolete) — — Splinter Review 
Observers are handled correctly, but failures are not handled properly (like in the old code).
Attachment #255653 - Attachment is obsolete: true
Yea this blocks bug 328070
Blocks: 328070
Flags: blocking1.9?
Not a regression from 2.0.  -'ing for 1.9. 

Polvi is cool.
Flags: blocking1.9? → blocking1.9-
do we really want to ship with this bug?
Blocks: 411215
> do we really want to ship with this bug?

We shipped 1.5 with document.loadOverlay in very questionable state, so it's probably not such a big deal.

More to the point, this code is rather messy and doesn't care about *so* many cases, that making sure it works correctly in all code paths is quite hard.

It would be easy to cook up a patch to make successive loadOverlay calls work properly in the common cases, but then you're replacing a clear bug (only one overlay can be loaded at a time) with lots of subtle bugs and the risk to introduce security risks.

(BTW I'm not working on this now and probably lost the more recent patch than what I have attached, so feel free to steal this one from me. You have been warned, though!)
The problem is that apparently this prohibits pefpanes (which are loaded with this function) from being overlayed at all, which does not only reduce extensibility (which we probably want) but also would be something that SeaMonkey needs in our new toolkit-derived prefwindow: Once I overlay (with a conventional manifest-based overlay) the pane that's being loaded with loadOverlay(), the observer to that loadOverlay() call doesn't fire any more.
> We shipped 1.5 with document.loadOverlay in very questionable state, so it's
> probably not such a big deal.

Sad to say, but Toolkit's prefwindow definitely isn't quite ready for realworld usage outside of simple cases like Firefox' prefpanes. See also bug 410562 and others.

> More to the point, this code is rather messy and doesn't care about *so* many
> cases, that making sure it works correctly in all code paths is quite hard.
> 
> It would be easy to cook up a patch to make successive loadOverlay calls work
> properly in the common cases, but then you're replacing a clear bug (only one
> overlay can be loaded at a time) with lots of subtle bugs and the risk to
> introduce security risks.

Shouldn't it suffice to just queue the overlays and then walk back notifying the observers? You'd definitely don't want to load an overlay twice to avoid looping, but (simple) ested overlaying should work.

Which security risks do you expect? (While of course always possible, this pretty much sounds like a scarecrow here.)

I remember very little about this code, so can't delve into technical discussion now. I wrote what was my impression from trying to patch up this code.

> Shouldn't it suffice to just queue the overlays
That's what I'm doing. The hard thing about this code is that 
a) the call graph is quite complicated on its own (supporting many different scenarios - fastload vs no fastload, initial load vs dynamic overlay, etc)
b) certain callbacks happen asynchronously from the event loop (meaning that the document may be touched in the interim)
c) it used to be the case that certain spots in the code could cause scripts to run (which could cause our code to be reentered).

You have to be sure you get to the end of the load process in any case - to call all overlay observers. And you have to be sure you handle the async notifications properly whatever order they come in (and be some notification always happens, so that you don't live in the loading state forever).

With the WIP#2 patch (I think) I had the problem with cancelled (BINDING_ABORTED) loads not being processed correctly; in normal cases it seemed to work correctly, but touching this code causes paranoia, as you see.

re scarecrow: possibly, but overlays are available to content, and the reentrancy issues and the fact that the script is able to make unexpected changes to document's state while it is loading make me feel uncomfortable. I'm mainly afraid of crashes here.

Maybe you're right and we should implement the simple fix and deal with corner cases in separate bugs. Feel free to take it :) I'll try to see if I still have my tests somewhere (not until Monday probably).

A patch like this feels like an alpha material though.

Unbitrotted to the extent that it builds and appears to work - only to be considered as a starting point for the moment.
Attachment #258243 - Attachment is obsolete: true
Attached file different testcase, v1 — 
Extract the contents of the zip into the extensions folder of a trunk nightly of SM or FF and open
  chrome://testcase330458/content/

With patch attachment 303723 [details] [diff] [review] applied, each group has three green labels. Without it, the last two groups don't load completely and stay red.

Nickolay, any ideas how to test this BINDING_ABORTED problems you saw?
Stuffing the overlay files with useless comments (eg. until they're 10 MB) allows to interrupt the loadOverlay calls. In this case, the testcase will fail - but I wouldn't expect it to work in this case anyway. 
I didn't see any suspicious behaviour either, so maybe the testcase is just too simple for testing such edge case. Suggestions?

Otherwise, I'd suggest asking for first reviews...
Comment on attachment 303723 [details] [diff] [review]
Unbitrotted WIP patch v0.3

Okay, going for reviews then...
Attachment #303723 - Flags: superreview?(bzbarsky)
Attachment #303723 - Flags: review?(jst)
Comment on attachment 303723 [details] [diff] [review]
Unbitrotted WIP patch v0.3

Neil, I think you'd be a better reviewer for this than I would...
Attachment #303723 - Flags: review?(jst) → review?(enndeakin)
Sorry, didn't find the tests. The binding_aborted thing was reproducible by stopping the load (e.g. using window.stop) after the loadOverlay call, iirc.

Thanks for taking this, I'd suggest fixing the XXXs (the stylistic one and notification-related at least) and poor grammar in comments before asking for reviews though.
Assignee: asqueella → nobody
Status: ASSIGNED → NEW
Grammar fixes can be applied once we know whether the actual code changes are okay or not. :)
Assignee: nobody → mnyromyr
I doubt I'll be able to sr this in any sort of reasonable timeframe (most likely, not until June at best).
Blocks: 398751
Bug 415152 which is now duped against this one has blocking-firefox3+. Reasking for blocking-1.9.
Flags: blocking1.9?
Discussed with mconnor & beltzner regarding the blocking status of Bug 415152.  That bug is a P4 and is being moved to the next 0.x release.  Marking this bug accordingly.

Also, can't reproduce.  Anyone have more info?
Flags: wanted1.9.0.x+
Flags: tracking1.9-
Flags: blocking1.9?
Flags: blocking1.9-
(In reply to comment #27)
> Discussed with mconnor & beltzner regarding the blocking status of Bug 415152. 
> That bug is a P4 and is being moved to the next 0.x release.  Marking this bug
> accordingly.
> 
> Also, can't reproduce.  Anyone have more info?

I can reproduce 100%, unfortunately.  If you can reproduce, would you restore blocking?
(In reply to comment #27)
> Also, can't reproduce.  Anyone have more info?  

It's very easy to reproduce. Open the preferences panel and hold down the left or right arrow key to immediately switch as fast as possible through the several panes. Do this for one cycle. Afterwards you will see the error message within the Error Console. Tested with OS X 10.4.11.
Comment on attachment 303723 [details] [diff] [review]
Unbitrotted WIP patch v0.3

Why is a review requested on this patch if its author does not think it is ready?
No longer blocks: 398751
Flags: blocking1.9- → blocking1.9?
Attachment #303723 - Flags: superreview?(bzbarsky) → superreview?
Is this a regression?  (It doesn't look like it is.)  If not, what's the case that it actually *blocks* the release?
Seems like a regression to me, since I can't reproduce anything like this in Firefox 2 on Mac.

https://bugzilla.mozilla.org/show_bug.cgi?id=330458#c29 has the steps, which are simple to follow.

Case that it *blocks* the release?: when switched to after following the above steps, the preference panels' contents will be empty until the window is closed and reopened.  I had as many as three panels in this state while testing.

(Additionally, there is a smattering of embedding issues which I don't really claim to fully understand.)
This core bug is not a regression, since the bug is basically about the overlay queueing code that never was written.

The case for blocking is platform quality, I suppose. Without this bug fixed seamonkey folks will have additional trouble implementing their preferences the way they want it. And this introduces an additional way for extensions to conflict.
(In reply to comment #32)
> Seems like a regression to me, since I can't reproduce anything like this in
> Firefox 2 on Mac.

It's not a regression. It basically never worked.
Flags: wanted-next+
Flags: blocking1.9?
Flags: blocking1.9-
(In reply to comment #34)
> (In reply to comment #32)
> > Seems like a regression to me, since I can't reproduce anything like this in
> > Firefox 2 on Mac.
> 
> It's not a regression. It basically never worked.

I should've been more clear; my originally filed bug, bug 415152, which was duped to this (and had blocking before), is clearly a regression from Firefox 2 -> 3 on Mac.
(In reply to comment #35)
> I should've been more clear; my originally filed bug, bug 415152, which was
> duped to this (and had blocking before), is clearly a regression from Firefox 2
> -> 3 on Mac.

Sadly the latter one isn't true. I used the described STR from my comment 29 and it is reproducible with the latest 1.8.1 nightly build. At least two panels were empty.
I run Firefox 3.0 on Linux, and this seems to be the problem I ran into with certain preferences panes just the other day.
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
Blocks: 452197
This problem causes the problem for  dynamic loading of editBookmarkOverlay.xul. 

When the timing of clicking the star button and other dynamic loadings  is almost same time,  "Edit Bookmarks panel" is not displayed. 

This is a very serious problem.
No longer blocks: 452197
Depends on: 452197
Blocks: 452197
No longer depends on: 452197
Flags: wanted1.9.0.x+ → wanted1.9.1?
Flags: wanted1.9.1? → wanted1.9.1+
Attachment #303723 - Flags: superreview?
Attachment #303723 - Flags: review?(enndeakin)
In the documentation on https://developer.mozilla.org/en/Document.loadOverlay, it tells that we have to rely on observer to know when the loading is complete. But when I try to load an overlay, and then load a second overlay inside the observer::observes method, it seems like second signal is never fired even if the second overlay is loaded. (I can see the xul code with a dump of the xul document).

Currently, I try to load two overlays with this pattern:
1. call the loading function
   1.1 loadOverlay(url_one, new observer)
2. observer::observe is called with the good signal
   RESULT = the document 1 is loaded
   2.1 recall the loading function
   2.2 loadOverlay(url_two, new observer)
3 <observer.observer is never called>
   RESULT = the document 2 is loaded, but the event was not fired

I'm trying to use loadOverlay because I have a system that work with modules (actived depending on user rights known on runtime after a login dialog). Rights now I have the choice to load all overlays (wait about 30 seconds for xulrunner to open) or open the core system (wait around 10 seconds).

So, can you paste me some code that will help me to understand how to load more than 1 overlays?
The following code can load more than 1 overlays.
usage :
myLoadoverlay.loadOverlay(urlA, your_observerA, null);
myLoadoverlay.loadOverlay(urlB, null, null);
myLoadoverlay.loadOverlay(urlC, your_observerC, null);

Will run in the following order:
load urlA, execute observerA, load urlB, load urlC, execute observerC


//myLoadoverlay.loadOverlay(aURL, aObserver, document)
var myLoadoverlay = {
    overlayWait:0,
    overlayUrl:[],

    loadOverlay: function(url, observer, doc) {
       myLoadoverlay.overlayUrl.push([url, observer, doc]);
       if (!myLoadoverlay.overlayWait) 
         myLoadoverlay.load(++myLoadoverlay.overlayWait);
    },

    load: function() {
        if (!myLoadoverlay.overlayUrl.length)
          return --myLoadoverlay.overlayWait;
        var [url, aObserver, doc] = this.overlayUrl.shift();
        if (!doc)
          doc = document;
        if (!aObserver)
          aObserver = myLoadoverlay.observer;
        else {
          var func = aObserver.observe.toString();
          func = func.replace(/{/,"{if (arguments[1] == 'xul-overlay-merged') {myLoadoverlay.load();}");
          eval("aObserver.observe = " + func);
        }
        var original_loadOverlay = Components.lookupMethod(doc, 'loadOverlay');
        try {
          original_loadOverlay(url, aObserver);
        } catch(ex) {
          myLoadoverlay.error(url, ex);
        }
        return 0;
    },

    observer: {
      observe : function (subject, topic, data) {
          if (topic == 'xul-overlay-merged') {
            myLoadoverlay.load();
          }
      },

      QueryInterface: function(aIID){
          if(!aIID.equals(Components.interfaces.nsISupports)
             && !aIID.equals(Components.interfaces.nsIObserver))
            throw Components.results.NS_ERROR_NO_INTERFACE;
          return this
      }
    },

    error: function(aMsg, err){
      const CONSOLE_SERVICE = Components.classes['@mozilla.org/consoleservice;1']
                              .getService(Components.interfaces.nsIConsoleService);
      var error = Components.classes['@mozilla.org/scripterror;1']
                              .createInstance(Components.interfaces.nsIScriptError);
      if (typeof(err) == 'object')
         error.init(aMsg + '\n' + err.name + ' : ' + err.message, 
                    err.fileName ||
                    null, null, err.lineNumber, null, 2, err.name);
      else
        error.init(aMsg + '\n' + err + '\n', null, null, null, null, 2, null);
      CONSOLE_SERVICE.logMessage(error);
    }
}
Sorry for the latest comment, but I tested more, created a test case on 
http://test.progysm.com/xul/overlay/window.xul that works when you click on button 2 and found why it doesn't work inside my code.

My second overlay has 3 overlays.
When you try to load overlay that have overlays, it doesn't fire the merge signal.
(In reply to comment #42)
I think that it is not the contents about which it should argue here. 
Please argue by the following forums. 
http://forums.mozillazine.org/viewforum.php?f=19
I have found that if the overlay references a stylesheet with the <?xml-stylesheet ... ?> link then any observer based queuing system loads the first overlay and then fails to load subsequent items in the queue (I tried my own one and also the one above written by Alice White). If I take out the stylesheets it works. It would seem that Yan Morin is correct when he comments about signals not being fired when you would expect them to be. I don't know what Mozilla's policy is on overlays but I would hope that the observer would be triggered with a success/failure result regardless of what stylesheets or other overlays were referenced in the one you tried to load.

I'm not willing to keep the CSS for each overlay in the main stylesheet for my Firefox extension and the only way I've found so far to load multiple overlays in a specific sequence, each with their own CSS and JS files, is to use the timer based technique shown below. The presence of the loaded overlay is detected by checking for an element known to exist in the overlay. The observer is not used as it's useless in the current state of affairs.

var overlay_loader={

  queue: [],
  loading: false,

  add: function(url,id)
    {
    this.queue.push([url,id]);
    },

  load: function()
    {
    if(!this.loading)
      {
      this.loading=true;
      document.loadOverlay(this.queue[0][0],this);
      }
    else
      {
      if(document.getElementById(this.queue[0][1])!=null)
        {
        this.loading=false;
        this.queue.shift();
        if(this.queue.length==0)return;
        }
      }
    setTimeout('overlay_loader.load()',100);
    },

  observe: function(subject,topic,data)
    {
    // don't bother to do anything because we cannot
    // guarantee that this observer will be called anyway
    }
  }

You would use this in the following way:

overlay_loader.add('chrome://example/content/main.xul','category_selector');
overlay_loader.add('chrome://example/content/book.xul','book_details');
overlay_loader.load();
Erik: please file that issue (observer not firing) as a separate bug. Most likely, it's an issue that needs to be dealt with before this bug is fixed, since the fix to this bug would likely just implement the queuing system inside document.loadOverlay().
Karsten, do you wanna still be the assignee of this bug?
Not working on this.
Assignee: mnyromyr → nobody
So how can we proceed with this bug? It's laying around for a couple of years now.

In case of QAE it blocks us from writing a couple of Mozmill tests because the preferences dialog produces some unpredictable behavior. See bug 499691. We don't use observers but whether which timing we are using tests will timeout because panes don't get loaded and displayed.
Flags: blocking1.9.2?
> So how can we proceed with this bug?

Find someone who has time on their hands to deal with the completely broken crap that is loadOverlay?
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Flags: wanted1.9.2+ → wanted1.9.2-
modified version of Erik's one to enable multiple load calls : 

var OverlayLoader = {
  queue: [],
  loading: false,

  add: function(url,id)
  {    
    OverlayLoader.queue.push([url,id]);
  },

  load: function()
  {
    if (OverlayLoader.queue.length == 0) return;
    if (!OverlayLoader.loading)
    {
      OverlayLoader.loading = true;      
      document.loadOverlay(OverlayLoader.queue[0][0], null);
    }
    else
    {
      if (document.getElementById(OverlayLoader.queue[0][1]) != null)
      {  
        OverlayLoader.loading = false;
        OverlayLoader.queue.shift();
        if (OverlayLoader.queue.length == 0) return;
      }
    }
    setTimeout('OverlayLoader.load()', 100);
  }
};

overlay_loader.add('chrome://example/content/main.xul','category_selector');
overlay_loader.load();

----

overlay_loader.add('chrome://example/content/book.xul','book_details');
overlay_loader.load();


usefull for us, when loading overlays in more than one extension at the same time.
(In reply to comment #51)

>       document.loadOverlay(OverlayLoader.queue[0][0], null);
...
>     setTimeout('OverlayLoader.load()', 100);

Using a timeout here seems the very wrong way to go. Why don't you use an
observer as second argument to your loadOverlay() call? That way, you can
really know when the overlay is merged and pop another one from the queue.
(In reply to comment #52)
> Why don't you use an observer as second argument to your loadOverlay() call?

If you look at comment 44 you'll see that I mentioned the problem whereby the observer is not fired if the overlay contains XML references to other overlays (stylesheets, scripts or other overlays).
(In reply to comment #53)
> If you look at comment 44 you'll see that I mentioned the problem whereby the
> observer is not fired if the overlay contains XML references to other overlays
> (stylesheets, scripts or other overlays).

(Which you filed as bug 496320, but never mentioned here, so it was never looked at.)
Is this bug still exists in version 24???
MDN still referring to it
https://developer.mozilla.org/docs/Web/API/document.loadOverlay#Specification
Is this bug now WONTFIX?
Flags: needinfo?(bdahl)
Yes.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(bdahl)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: