19421 - No menubar on Mac -- brutal JS script sharing bug?

Status: VERIFIED FIXED

(SeaMonkey :: General, defect, P3)

Description

[Pierre Saslawsky]

Simon said:
From a build of around 8pm Thurs, I'm seeing a complete lack of menu bar
on Mac, even after closing the first browser window. It kinda makes the
app hard to use.
--
I confirm that the problem still exists with a build from this morning.

Comment 1

[Pierre Saslawsky]

If you close the first window, the menubar appears and remains for the rest of
the session even after selecting New Window.

Comment 2

[Simon Fraser [no longer active]]

What I'm seeing is that the OnEndDocumentLoad for navigator.xul is being called
before that for hiddenWindow.xul, which creates ordering problems for menu
contruction. No leads here yet.

Comment 3

[Daniel (Leaf) Nunes]

I think saari has a better shot at figuring this out than jan does ;)

Comment 4

[Simon Fraser [no longer active]]

This is looking very much like the brutal JS script sharing changes are to blame.
We are hitting the assertion at line 4589 of nsXULDocument.cpp:

NS_ASSERTION(!mCurrentScriptProto, "still loading a script when starting another
load?");

Builds from the morning of 11/18 show the problem (the day after brutal JS
sharing went in).

We suspect that the problem is that on Mac, we're doing two overlapping XUL file
loads, and that this is somehow screwing up the brutal sharing prototype
construction. The same problem would likely be apparent on Windows if you did
overlapping loads of nav.xul.

Comment 5

[Simon Fraser [no longer active]]

brendan sez that he'll need waterson's help to fix this; it's the same brutal
sharing problem for overlays that exists for the master document. For now, we've
disabled the brutal sharing of scripts to work arouind the problem. /be will
explain.

Comment 6

[Brendan Eich [:brendan]]

Ok, here's what I think is happening, based in part on sfraser and saari's
helpful printfs:

Mac loads hiddenWindow.xul, which loads stuff including globalOverlay.xul, which
loads globalOverlay.js.  It also loads navigator.xul in the first window; this
load starts some time after hiddenWindow's load has initiated, but before it has
completed, due to pseudo-threading with the filechannel worker thread and XUL
layout state-machine blocking.

The XUL code brutally shares globalOverlay.xul, and my code tries to share
globalOverlay.js, but my code didn't execute that compiled script for the racing
loser load-waiter (from navigator.xul via globalOverlay.xul), which led to the
assertbotch sfraser saw.  But there's more: since the racing loser didn't go
through necko to load globalOverlay.js, no load event accounting was done for
that subdocument.  I believe there was likewise no load event accounting for
globalOverlay.xul, and sfraser indeed saw the load event for navigator.xul fire
early, before the one for hiddenWindow.

That's the same problem that kept waterson and hyatt from brutally sharing the
master document (navigator.xul for two browser windows); here it's biting the
brutal sharing of an overlay (globalOverlay.xul), but the heretofore necko-based
loading of .js files hid that, and preserved load event order.

My changes of the other night, to enable brutal script sharing, exposed the
underlying bug and left the load event firing when the file worker thread said
it was done reading navigator.xul.  I need waterson's help to figure out how to
fire load events from XUL code after properly accounting for layout
completion of all brutally-shared sub-documents.

/be

Comment 7

[Brendan Eich [:brendan]]

Adding dependency on 18392, which wants load-event accounting for brutally
shared master XUL docs.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: rev 1.198 patch to avoid assertbotch and fix racing-loads logic (not complete fix for this bug)

Comment 9

[David Hyatt]

I don't think this is the same problem, and I don't think it should be solved
the same way as the master doc problem.

It seems like the only way this would really happen is if waterson is caching
the globalOverlay BEFORE it has actually finished parsing.  In other words, the
second document (navigator.xul) must be pulling a globalOverlay.xul out of the
cache that is incomplete.

It seems like this problem could be bypassed by not allowing brutal sharing of
XUL files to occur until you know they've been fully loaded, e.g., in this
disgusting case where two docs are loaded at the same time, just let them BOTH
load globalOverlay.xul.  Don't allow the overlay to be placed into the cache
until it's truly ready to be shared.

Comment 10

[Brendan Eich [:brendan]]

(Tell me if we should take this to mail or a newsgroup -- I'm happy to stay in
the bug report.)

The problem is not that waterson caches globalOverlay.xul before it's complete.
He fills (nsXULPrototypeCache::PutPrototype) from nsXULDocument::EndLoad.  The
problem is that when he hits the cache (gXULCache->GetPrototype in ResumeWalk),
he walks it to lay out the current (second or later) XUL document but *does not
account for load events via the usual LoadGroup mechanism*.

So as soon as the one thing going through the necko LoadGroup mechanism, the
master document, finishes, a load event fires early.  That's what happened last
night with sfraser driving and me on the phone, with my changes to share script
instead of reloading when racing to load a script.  Without my change, we do as
you propose: we make the losing racer reload through necko.  This delays load
event firing until the .js file is fetched, which happens to be long enough for
the content iframe to be created.

But the lack of load-group-based load event accounting when hitting the cache,
whether for overlays or for the master document, is still the bug I think we
need to fix.

/be

Comment 11

[Brendan Eich [:brendan]]

BTW, "this disgusting case" of racing loads for the same overlay or script file
is not uncommon: besides hiddenWindow vs. first navigator.xul load on Mac, saari
could make it happen with File / New Browser Window if he was fast enough (kbd
shortcuts would make this race even likelier -- I do Ctrl-N;Ctrl-N bang-bang
fast myself, on 4.x).

/be

Comment 12

[saari (gone)]

*** Bug 19240 has been marked as a duplicate of this bug. ***

Comment 13

[saari (gone)]

*** Bug 19244 has been marked as a duplicate of this bug. ***

Comment 14

[Chris Waterson]

So let me get this straight. The problem is that 2nd loader of globalOverlay.js
detects that 1st loader is already loadng this. He blocks; i.e., returns to the
main event loop, without adding first loader's URL to his document's load
group. Docloader therefore fires endload early.

If that is the case, it is indeed ugly. Here are some ideas.

1. Can we add the first doc's load to the second doc's load group? I imagine
that a URL load cannot be part of two load groups.

2. Can we expose an API on the docloader to increment the wait count?

I think I like (2) better.

Comment 15

[Brendan Eich [:brendan]]

If our goal is to fix 18392 and hit the XUL prototype cache for the master doc
too, we'll want to fire nsWebShell::OnEndDocumentLoad or something like it after
walking the prototype doc-tree and sharing it brutally.  In that "total cache
hit" scenario, we won't have gone through necko for anything.  Will we even have
a docloader (the webshell's docloader) whose wait-count we could bump?

/be

Comment 16

[Brendan Eich [:brendan]]

Attachment: patch to top of trunk, last patch was also buggy (used mCurrentScriptProto after it had been cleared, instead of using scriptProto local -- d'oh!). Still need to fire load event at end of walk that clones a prototype

Comment 17

[Chris Waterson]

The patch looks good, and makes sense, but I don't see how we'll avoid firing
OnEndDocumentLoad early. We still need to tinker with the docloader, right?

Comment 18

[Brendan Eich [:brendan]]

Yes, we need to tinker with the docloader when there is a master docloader who
can gate load events.  When there isn't one (i.e., when we're brutally sharing
the master doc via the prototype cache), then we need a latch that closes at the
end of ResumeWalk, firing the load event from there -- right?

Hope I'm not on crack.  If not, the above sounds do-able today, and I'd like to
get this fixed by tomorrow.

/be

Comment 19

[Chris Waterson]

*** Bug 19241 has been marked as a duplicate of this bug. ***

Comment 20

[Chris Waterson]

checked in brendan's patch, plus a ``placeholder channel'' that ensures we don't
fire the end document load prematurely.

Comment 21

[leger]

Setting QA contact.

Comment 22

[Brendan Eich [:brendan]]

*** Bug 19165 has been marked as a duplicate of this bug. ***

Comment 23

[Claudius Gayle]

VERIFIED fixed for 1999112308 build