1001957 - [e10s] Confirm repost dialog not working

Status: RESOLVED FIXED

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

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The code is here:
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#11837

When the dialog is supposed to be displayed, everything gets broken and I have to restart the browser.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

It turns out billm already had a patch for it. If it passes try <https://tbpl.mozilla.org/?tree=Try&rev=ff20d2da1c32> I'll attach it here and get it into the tree.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Don't use a CPOW for reload.

The underlying cause for this bug is that "reloading" currently uses a CPOW to communicate from the parent to the child process. When docshell then wants to put up a modal prompt, we send a message to the parent from the child and then wait for a response, but we're blocked on the CPOW call.

There doesn't appear to be a strong reason for the CPOW use here, so this patch (written originally by billm) makes us use an async message.  It also has to fix a test that does assume that reloading is synchronous and there's one other consumer that mochitests found that I'll attach another patch for in a second.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Update the handling user input state correctly.

Accessibility relies on the state of the ESM's "handling user input" flag for its events. Now that we're using an async message, we have to pass that through the event and set things up on the other side to make things work correctly. It's a little scary that we have to do this, and I'm not thrilled by this, but I can't find a better solution.

Andrew, would you mind reviewing the DOM bits?

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8431212 [details] [diff] [review]
Update the handling user input state correctly.

Review of attachment 8431212 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMWindowUtils.cpp
@@ +3919,5 @@
>  
>    return style->GetPropertyValue(aProperty, aResult);
>  }
>  
> +namespace {

Is this |namespace| just to keep the class definition from escaping this file?

@@ +3938,5 @@
> +};
> +
> +NS_IMPL_ISUPPORTS(HandlingUserInputHelper, nsIJSRAIIHelper)
> +
> +HandlingUserInputHelper::HandlingUserInputHelper(bool aHandlingUserInput)

It seems a little weird that this does nothing if you pass in false, rather than just requiring that somebody create a dummy destruct() object in the JS layer, but okay.

@@ +3971,5 @@
> +
> +  return NS_OK;
> +}
> +
> +}

Some comment saying that you are closing the empty namespace thing?

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1663,5 @@
>     AString getOMTAOrComputedStyle(in nsIDOMElement aElement,
>                                    in AString aProperty);
>  
>     /**
> +    * If we are currently handling user input, this informs the event state

I think the description should be more like, if you pass in true, it informs the event state manager, or something, as it isn't checking anything.  And it would be nice if you were more explicit that otherwise it doesn't really do anything.

@@ +1664,5 @@
>                                    in AString aProperty);
>  
>     /**
> +    * If we are currently handling user input, this informs the event state
> +    * manager. Remember to call destruct on the return value!

destruct -> destruct() maybe?

@@ +1708,5 @@
>  };
> +
> +/**
> + * JS doesn't do RAII very well. We can use this interface to make remembering
> + * to destruct an object in a finally clause easier.

Eww...

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Don't reload the page in the middle of running tests.

This was causing mochitest-2 orange. I looked for a while but I couldn't figure out why the patch in this bug changed the behavior here, I wonder if we cancel a reload of a page if a subframe navigates? I debugged in the pre-patch version to verify that we were, in fact, calling nsDocShell::LoadURI (which we were) but didn't go further.

This patch simply avoids propagating the event all the way up to chrome.

smaug, I don't know who owns the browser element code any more, you were the likeliest candidate.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8432015 [details] [diff] [review]
Don't reload the page in the middle of running tests.

kanru owns mozbrowser.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> Is this |namespace| just to keep the class definition from escaping this
> file?

Yeah. I don't know if it actually helps, but my understanding is that this makes the class definition be non-extern and potentially faster/reduces the number of symbols in the global namespace.

> It seems a little weird that this does nothing if you pass in false, rather
> than just requiring that somebody create a dummy destruct() object in the JS
> layer, but okay.

I thought about this, but decided that it was nicer to let the JS code look the same whether or not we're actually handing user input.

I addressed the rest of your comments.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8431211 [details] [diff] [review]
Don't use a CPOW for reload.

Can you explain how the test change removes the assumption that reloading is synchronous? Not sure I follow.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8431212 [details] [diff] [review]
Update the handling user input state correctly.

Which test was this causing to fail?

It seems likely that we'll need a more generic mechanism for this (tagging messages with "handling user input" state automatically without the participation of the sendAsyncMessage caller?), but I don't know enough about how much this is used.

Perhaps you could file a followup?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

I've been on vacation. I'll get to this tomorrow.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #9)
> Can you explain how the test change removes the assumption that reloading is
> synchronous? Not sure I follow.

The test assumed that by the time we ran that code, we were guaranteed to have parsed the document (I don't remember offhand which property get in the chain was null but I think content.document was non-null but the getElementById was returning null). That was the "synchronous" part of the assumption. Now, we correctly wait until the document is fully set up.

(In reply to :Gavin Sharp (email [e-mail]) from comment #10)
> Which test was this causing to fail?

accessible/tests/mochitest/events/test_docload.xul

> It seems likely that we'll need a more generic mechanism for this (tagging
> messages with "handling user input" state automatically without the
> participation of the sendAsyncMessage caller?), but I don't know enough
> about how much this is used.

It's hard to tell how much this is the case. Most users of the ESM state should be actually handling user input when they check. I'd prefer to land this and do something general if anything else pops up.

> Perhaps you could file a followup?

I could definitely do that.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8431212 [details] [diff] [review]
Update the handling user input state correctly.

Felipe, can you take a look or suggest a better reviewer since Gavin's on vacation?

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76a96d6f0330

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

I spent another few minutes re-tracking down why the change to browser_bug822367.js was needed. I believe the reason is that now that we do the reload on an event instead of synchronously, the test now allows the inner iframe to load and do document.open, which fires onload, triggering MixedTest6C which starts trying to use the outer document as its being reloaded. Before the patch here, we would have blocked the load entirely. I don't believe that this would cause a problem in the real world.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

I filed bug 1036154 for the general solution to the IsHandlingUserInput problem.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for causing mochitest-2 failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=43384438&tree=Mozilla-Inbound


https://hg.mozilla.org/integration/mozilla-inbound/rev/fbb2bdecd50d

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Sorry about that. I didn't include the third patch here in my first push :-/

Let's try again, but this time with the whole patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/d35be691e56f

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d35be691e56f

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3e052ab5a27

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d3e052ab5a27