1124122 - Event suppression doesn't seem to work with postMessage

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Shu-yu Guo [:shu]]

I'm running into a case where debugger; statements are not hit until after quite a bit. Preliminary debugging shows that the onDebuggerStatement hook is actually being hit, but we are unable to pause because the ThreadActor.state is already "paused" from the initial pause.

Debuggee JS shouldn't be able to run during this time -- best guess after talking with Eddy and Panos on IRC is that event suppression isn't working right for iframes.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: test.html

The iframe was a red herring. It's really postMessages being pumped even with event suppression. If you run this test with the debugger open and check the console, one should notice several "AFTER DEBUGGER" messages being hit before we actually pause in the Debugger on the |debugger;| statement.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Shu-yu Guo [:shu] from comment #0)
> best guess after
> talking with Eddy and Panos on IRC is that event suppression isn't working
> right for iframes.

Made a test page to verify this:

http://htmlpad.org/paused-iframes/

Opening the debugger and hitting pause will stop execution in both iframe and parent window (=timeout is suspended). Double-clicking on the iframe won't select the text (=events are suppressed). So the iframe was indeed a red herring.

Comment 3

[Shu-yu Guo [:shu]]

So the problem, as best as I understand, after talking with mrbkap, is this:

The frontend's "pushThreadPause" mechanism is fundamentally broken.

We attempt to pause debuggee code by 1) suppressing events on the content window and 2) suppressing timeouts on the content window. Afterwards, we spin a nested event loop, with the intention of processing debugger chrome code. The event loop itself isn't intrinsically hierarchical. A nested event loop still pumps the *same* message loop. Indeed, there's a single message loop per process.

Neither 1) nor 2) above actually pauses the debuggee code. Things like postMessage slip through, as do other things on the message loop that aren't events that event suppression in 1) above suppresses. This means that we simply do not have a guarantee that debuggee code does not run while the frontend debugger is visually paused.

I guess a new DOM API is needed that tells a document to just "stop", so all messages on it are enqueued and saved up somewhere. This is tricky, as mrbkap pointed out, because we also have to consider all other similarly-origined windows. Those other windows can hold CCWs to functions inside our "stopped" document, and can thus invoke code in our debuggee document directly.

I think this should be high priority, because it erodes confidence in users. Imagine if you started up gdb and your breakpoints just don't hit for the first 2s. How can you trust in the rest of the toolkit?

Comment 4

[Shu-yu Guo [:shu]]

Most realistic way forward, since per-document message queues is pretty much shangri-la, is to add a new API to suppress and resume postMessage events. The current event suppression on the pres shell (or was it something else?) basically only suppresses focus events.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Shu-yu Guo [:shu] from comment #3)
> Neither 1) nor 2) above actually pauses the debuggee code. Things like
> postMessage slip through, as do other things on the message loop that aren't
> events that event suppression in 1) above suppresses. This means that we
> simply do not have a guarantee that debuggee code does not run while the
> frontend debugger is visually paused.

So, there are basically 2 ways that we can enter script in a given compartment:
(1) The system enters script (event dispatch, postmessage, etc)
(2) Script from another compartment invokes script in the given compartment via a cross-compartment wrapper (possibly indirectly via property manipulation).

I recently implemented an API that is supposed to handle (1), and is used by addons like NoScript, called BlockScriptForGlobal/UnblockScriptForGlobal. The debugger should almost use that instead of doing manual event supporession. And if there are holes in it (e.g. postMessage), we should fix it.

If (2) is a problem, we could solve it at the wrapper layer by temporarily sealing off all incoming wrappers while the debugger is paused.

I can't find the debugger's event suppression code, but it should almost certainly use the proper XPConnect API if it isn't already. Shu, are you interested in digging into this? I can help you out if so.

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #5)
> (In reply to Shu-yu Guo [:shu] from comment #3)
> > Neither 1) nor 2) above actually pauses the debuggee code. Things like
> > postMessage slip through, as do other things on the message loop that aren't
> > events that event suppression in 1) above suppresses. This means that we
> > simply do not have a guarantee that debuggee code does not run while the
> > frontend debugger is visually paused.
> 
> So, there are basically 2 ways that we can enter script in a given
> compartment:
> (1) The system enters script (event dispatch, postmessage, etc)
> (2) Script from another compartment invokes script in the given compartment
> via a cross-compartment wrapper (possibly indirectly via property
> manipulation).
> 
> I recently implemented an API that is supposed to handle (1), and is used by
> addons like NoScript, called BlockScriptForGlobal/UnblockScriptForGlobal.
> The debugger should almost use that instead of doing manual event
> supporession. And if there are holes in it (e.g. postMessage), we should fix
> it.
> 
> If (2) is a problem, we could solve it at the wrapper layer by temporarily
> sealing off all incoming wrappers while the debugger is paused.
> 
> I can't find the debugger's event suppression code, but it should almost
> certainly use the proper XPConnect API if it isn't already. Shu, are you
> interested in digging into this? I can help you out if so.

Sure, I can fix this if the new API is suitable. Does BlockScriptForGlobal *delay* the events until the Unblock call or does it discard them? The debugger needs them delayed.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Shu-yu Guo [:shu] from comment #6)
> Sure, I can fix this if the new API is suitable. Does BlockScriptForGlobal
> *delay* the events until the Unblock call or does it discard them? The
> debugger needs them delayed.

It discards them - so if that isn't good enough, we need another story. But it probably makes sense to coordinate the APIs, because callsites checking whether script can run should be the same.

I think it makes sense to add another predicate to xpc::Scriptability called ::Delayed(). If caller support delaying execution, they can check ::Delayed() if ::Allowed() returns false.

For edge cases where delaying isn't supported, do we want to default to running script, or to not running it? I'm assuming the latter, but we should think about that.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #5)
> I can't find the debugger's event suppression code, but it should almost
> certainly use the proper XPConnect API if it isn't already.

The debugger actors call preNest() to pause execution before entering a nested event loop:

https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=preNest

BTA_preNest is the most common one, used when debugging a tab in Firefox desktop.

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I can still reproduce this bug:

Here are my STR:

1. Load http://janodvarko.cz/tests/bugzilla/1124122/
2. Open DevTools Toolbox and Browser Console
3. Reload the page
4. You should see 99 AFTER DEBUGGER logs in the Browser Console before breaking at the debugger statement

ER:
Debugger breaks on the debugger statement immediatelly.

AR:
The debugger statement is hit many times, but doesn't break the debugger.

Honza

Comment 10

[Brian Hackett [Laid off!]]

Attachment: Bug 1124122 - Suppress postMessage events on a window's document when event handling is suppressed.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51440617724b
Suppress postMessage events on a window's document when event handling is suppressed, r=smaug.

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/51440617724b