563850 - Add scriptable interfaces for EnterModalState et al.

Status: RESOLVED FIXED

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

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.0 (WIP)

Over in bug 563274 I'm a bunch of code from embedding's wwatcher component to a new JS component. To fully do this the JS code needs to be able to invoke nsGlobalWindow's EnterModalState() and LeaveModalState().

One slight trick to this is that these functions do some twiddling from GetCurrentJSContext(), which won't (?) be the right context when JS component code is invoking them. Johnny says bkap was working on changing this, so CC'd for advice. (In the meantime, I've just commented it out since it's not critical).

Comment 1

[Justin Dolske [:Dolske]]

(Oops, that should be s/mGlobalWindow/mWindow/)

Comment 2

[Justin Dolske [:Dolske]]

Last week mrbkap said that this should be fine with simply uncommenting the code that this patch commented out (calling scx->EnterModalState() / LeaveModalState). But I can't seem to recreate the testcase we used to try to validate that...

EG, when running this function in a web page:

function start() {
  var more = true;
  while (more) {
    // 3 second hang
    var t1 = Date.now();
    while (Date.now() - t1 < 3000)
        x++;

    more = confirm("Do more?");
  }
}

AIUI, this script _should_ trigger a slow script dialog after the 3rd or 4th dialog, since it'll have > 10 seconds of runtime. And that the point of the scx->[Enter|Leave]ModalState() code was to correct the accounting so that the script isn't penalized for the time spent in the modal dialog. Thus, with the patch, by waiting in the first dialog for 10 seconds I should get a slow-script dialog right after clicking ok.

However, neither a trunk nightly nor a build with Patch v.0 (ie, code commented out) ends up triggering a slow-script dialog... Doesn't matter how many times the main loop runs, or how long I wait in the dialog. Seems like something is broken (like the script's runtime gets reset to 0 on each loop), or I'm misunderstanding how this is supposed to work.

Upshot of all this is that I'm not sure how to write a testcase to ensure that this accounting remains correct when bug 563274 adds JS to the stack.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.0b (WIP)

Oops, previous v.0 patch doesn't apply. This one does.

Comment 4

[Justin Dolske [:Dolske]]

(In reply to comment #2)

> However, neither a trunk nightly nor a build with Patch v.0 (ie, code commented
> out) ends up triggering a slow-script dialog.

Did some debugging, the problem is that mOperationCallbackTime is being reset to 0 as some side effect of triggering a prompt. This seems to only happen in nsJSContext::ScriptEvaluated(), and indeed watching the variables address shows it being modified with a stack like:

3515	    mOperationCallbackTime = 0;
(gdb) bt
#0  0x008c1001 in nsJSContext::ScriptEvaluated (this=0x1e81c080, aTerminated=1) at dom/base/nsJSEnvironment.cpp:3515
#1  0x008b74fb in nsJSContext::ScriptExecuted (this=0x1e81c080) at dom/base/nsJSEnvironment.cpp:3584
#2  0x00d86438 in AutoScriptEvaluate::~AutoScriptEvaluate (this=0xbfffaa4c) at js/src/xpconnect/src/xpcwrappedjsclass.cpp:113
#3  0x00d8968a in nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject (this=0x1e8cb2c0, ccx=@0xbfffabdc, jsobj=0x162b8960, aIID=@0x16c33f8) at js/src/xpconnect/src/xpcwrappedjsclass.cpp:378
#4  0x00d8a929 in nsXPCWrappedJSClass::DelegatedQueryInterface (this=0x1e8cb2c0, self=0x1e8cb3a0, aIID=@0x16c33f8, aInstancePtr=0xbfffad6c) at js/src/xpconnect/src/xpcwrappedjsclass.cpp:797
#5  0x00d7f663 in nsXPCWrappedJS::QueryInterface (this=0x1e8cb3a0, aIID=@0x16c33f8, aInstancePtr=0xbfffad6c) at js/src/xpconnect/src/xpcwrappedjs.cpp:185
#6  0x01391a43 in nsWeakReference::QueryReferent (this=0x1e8cbb80, aIID=@0x16c33f8, aInstancePtr=0xbfffad6c) at nsWeakReference.cpp:151
#7  0x01391e29 in nsQueryReferent::operator() (this=0xbfffadb4, aIID=@0x16c33f8, answer=0xbfffad6c) at nsWeakReference.cpp:88
#8  0x0002cb91 in nsCOMPtr<nsIObserver>::assign_from_helper (this=0xbfffadc4, helper=@0xbfffadb4, aIID=@0x16c33f8) at nsCOMPtr.h:1249
#9  0x0002e5b6 in nsCOMPtr<nsIObserver>::nsCOMPtr (this=0xbfffadc4, helper=@0xbfffadb4) at nsCOMPtr.h:621
#10 0x013b103d in nsObserverList::FillObserverArray (this=0x4a30c84, aArray=@0xbfffadfc) at xpcom/ds/nsObserverList.cpp:106
#11 0x013b11ff in nsObserverList::NotifyObservers (this=0x4a30c84, aSubject=0x19772744, aTopic=0x18e3d78 "domwindowopened", someData=0x0) at xpcom/ds/nsObserverList.cpp:127
#12 0x013b22aa in nsObserverService::NotifyObservers (this=0x4407f70, aSubject=0x19772744, aTopic=0x18e3d78 "domwindowopened", someData=0x0) at xpcom/ds/nsObserverService.cpp:182
#13 0x00ebebfe in nsWindowWatcher::AddWindow
...
...nsPromptService...
...

(this is from a current trunk build, without the part of this patch that comments out stuff in nsGlobalWindow).

I didn't confirm it, but the observer that's run here is probably nsSessionStore's domwindowopened observer. I don't really understand why JS contexts are being reused for different scripts, so I'm not really sure how to fix this.

The good news for the interm is that:

1) This is already broken, but this patch won't seem to make it worse. Just can't test it. :(

2) It turns out that WindowWatcher is dumb, and calls Enter/LeaveModalState twice per window (via nsAutoWindowStateHelper): once in nsPromptService.cpp (for each of the nsIPromptService interfaces), and once in OpenWindowJSInternal(). 

I think bug 563274 can thus skip calling scriptable versions of these interfaces, and leave the work to OpenWindowJSInternal(). I'll still need the scriptable interfaces for the tab-modal prompts, though.

Comment 5

[Justin Dolske [:Dolske]]

(To clarify #1, I can't yet write a test to verify that the slow-script dialog is shown when appropriate, because it's not being shown. But it would be good to have a complementary test to verify that a slow-script dialog is _not_ triggered by having a prompt shown for a long time.)

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

I'd like to go ahead and get this in, will spin out the issues of the accounting already being broken to a new bug.

Comment 7

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/1ff4933ebe17

Filed bug 577385 on the (existing) broken accounting.