When two loads are executed in quick succession, e.g. loadUri("foo") then loadUri("bar"), we can have two pending onLoadUri calls on the Gecko stack, both waiting for a response from the UI thread: > (stack top) > ... > spinEventLoopUntil(onLoadUri("bar") responds) > handleLoadUri("bar") > ... > spinEventLoopUntil(onLoadUri("foo") responds) > handleLoadUri("foo") > ... In this case, it's clear that handleLoadUri("foo") would not return until after handleLoadUri("bar") returns, so "bar" ends up being processed before "foo". The result is an undesirable change in the order of loads. Normally, loadUri("bar") would force loadUri("foo") to cancel, but in this case it's the opposite -- loadUri("foo") forces loadUri("bar") to cancel.

This could be P1 IMO. It breaks page load behavior in non-deterministic ways.

Is it sufficient here to take the simple route of just "canceling" (pretending the app handled) any load that isn't the most recent in LoadURIDelegate as this patch does? Or do we want to go the whole distance of preserving the results of OnLoadRequest in the appropriate order?

Comment on attachment 8975534 [details] [diff] [review] Effectively cancel all but the latest LoadURIDelegate.load() call Review of attachment 8975534 [details] [diff] [review]: ----------------------------------------------------------------- If the consumer returns false for the first load, but true for the second load, the expected behavior is the first load proceeds as normal. But I think with this approach, the first load will _not_ proceed at all. We should do this properly and make the nsILoadURIDelegate interface asynchronous.

(In reply to Jim Chen [:jchen] [:darchons] from comment #3) > Comment on attachment 8975534 [details] [diff] [review] > Effectively cancel all but the latest LoadURIDelegate.load() call > > Review of attachment 8975534 [details] [diff] [review]: > ----------------------------------------------------------------- > > If the consumer returns false for the first load, but true for the second > load, the expected behavior is the first load proceeds as normal. But I > think with this approach, the first load will _not_ proceed at all. Yeah, that's a good point. > We should do this properly and make the nsILoadURIDelegate interface > asynchronous. Can you elaborate a bit on what you have in mind here? We rely pretty strongly on this being synchronous both for our implementation of nsIBrowserDOMWindow functions in GeckoViewNavigation.jsm and in nsDocShell::InternalLoad; it seems like either we will immediately have to block and wait for the result (in which case we don't really gain much compared to keeping LoadURIDelegate.load() synchronous), or we will have to do some fairly substantial rewriting upstream to accomodate nsILoadURIDelegate being async.

Yeah nsIBrowserDOMWindow probably has to stay synchronous for now, but it's possible to make nsDocShell::InternalLoad use nsILoadURIDelegate asynchronously. You would add a callback interface as a parameter of nsILoadURIDelegate::Load. InternalLoad would call Load with a callback object before returning. The callback object would save all of the arguments to InternalLoad. When the callback is invoked, it would call InternalLoad again with those saved arguments. nsDocShell already has an InternalLoadEvent class that calls InternalLoad asynchronously, so you can probably use that for this purpose.

After bug 1458258 and bug 1453501 this is the last issue we see frequently when restoring sessions and loading an URL from an Intent at the same time [in Fenix and Android Components].

This doesn't fix the issue at hand yet, just converts nsILoadURIDelegate to be async; for some reason I'm not getting the callback to the aEventDispatcher.sendRequestForResult() call in LoadURIDelegate.load(), which is what I'm currently trying to figure out.

Comment on attachment 8986487 [details] [diff] [review] async-loadUriDelegate.patch Review of attachment 8986487 [details] [diff] [review]: ----------------------------------------------------------------- I don't think you need `LoadURIDelegateCallback` if you make `InternalLoadEvent` implement `nsILoadURIDelegateCallback`, and you don't need `nsDocShell::InternalPreLoad` if you add a flag that tells `nsDocShell::InternalLoad` to not use `nsILoadURIDelegate`.

snorp, do you mind reviewing this since Jim's on PTO? I've made all of the changes he suggested in feedback and tested to confirm the patch fixes the issue, along with cleaning up the code quite a bit compared to the last patch.

Comment on attachment 8987613 [details] [diff] [review] Make nsILoadURIDelegate async Review of attachment 8987613 [details] [diff] [review]: ----------------------------------------------------------------- Please look into using a Promise instead of a callback interface. Also, we probably need a docshell person (bz, smaug) to look at this. ::: docshell/base/nsDocShell.cpp @@ +1019,5 @@ > aFirstParty, > srcdoc, > sourceDocShell, > baseURI, > + true, // Check for handlers I would maybe use "Check load delegates" or similar @@ +9162,5 @@ > , mLoadType(aLoadType) > , mFirstParty(aFirstParty) > , mSourceDocShell(aSourceDocShell) > , mBaseURI(aBaseURI) > + , mCheckDelegates(aCheckDelegates) mCheckLoadDelegates, perhaps? @@ +9197,5 @@ > + Notify(bool aHandled) override > + { > + if (aHandled) { > + return NS_OK; > + } else { No need for 'else' clause here ::: docshell/base/nsIDocShell.idl @@ +199,5 @@ > * @param aBaseURI - The base URI to be used for the load. Set in > * srcdoc loads as it cannot otherwise be inferred > * in certain situations such as view-source. > + * @param aCheckDelegates - Indicates whether we should look for an external > + * handler for the URI. "Indicates whether we should run the nsILoadURIDelegate that was set via 'loadURIDelegate'" ::: xpcom/base/nsILoadURIDelegate.idl @@ +41,1 @@ > loadURI(in nsIURI aURI, in short aWhere, in long aFlags, I think ideally this would return a Promise (jsval).

This incorporates all of snorp's recommended changes.

Comment on attachment 8988752 [details] [diff] [review] Make nsILoadURIDelegate async Review of attachment 8988752 [details] [diff] [review]: ----------------------------------------------------------------- This is looking pretty good but I think we should avoid adding arguments to InternalLoad() if we can. ::: docshell/base/nsDocShell.cpp @@ +9222,5 @@ > + > + LoadURIDelegateHandler(already_AddRefed<InternalLoadEvent>&& aInternalLoadEvent) > + : mInternalLoadEvent(aInternalLoadEvent) {} > + > + void whitespace ::: docshell/base/nsIDocShell.idl @@ +221,5 @@ > in boolean aFirstParty, > in AString aSrcdoc, > in nsIDocShell aSourceDocShell, > in nsIURI aBaseURI, > + in boolean aCheckLoadDelegates, I still like a flag for this instead. This method already has a billion arguments as it is. ::: mobile/android/modules/geckoview/LoadURIDelegate.jsm @@ +16,5 @@ > // Delegate URI loading to the app. > // Return whether the loading has been handled. > load: function(aWindow, aEventDispatcher, aUri, aWhere, aFlags) { > + return new Promise((resolve, reject) => { > + if (!aWindow) { You may find it a little nicer to do this check outside of the Promise callback. That way you can just `return Promise.resolve(false)` @@ +28,5 @@ > + where: aWhere, > + flags: aFlags > + }; > + > + aEventDispatcher.sendRequestForResult(message).then(response => { Ah, you actually don't need to construct a new Promise at all. Just chain off of this one. return aEventDispatcher.sendRequestForResult(message).catch(() => { return false; });

I assume this is somewhat urgent and shouldn't wait on Kyle Machulis's refactoring of InternalLoad? In either case, please coordinate with him a bit...

I'm ok with a load flag here, as recommended in Comment 12. That'll still fit fine with the internalLoad changes I'm making in Bug 1471711.

(In reply to Kyle Machulis [:qdot] [:kmachulis] (if a patch has no decent commit message, automatic r-) from comment #14) > I'm ok with a load flag here, as recommended in Comment 12. That'll still > fit fine with the internalLoad changes I'm making in Bug 1471711. Alright, will do. Let me know if you need any other changes.

Updated to use a load flag instead of an extra param for InternalLoad; also fixes snorp's nits.

Comment on attachment 8988877 [details] [diff] [review] Make nsILoadURIDelegate async Review of attachment 8988877 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/nsDocShell.cpp @@ +9485,5 @@ > > const bool isDocumentAuxSandboxed = doc && > (doc->GetSandboxFlags() & SANDBOXED_AUXILIARY_NAVIGATION); > > + const bool checkLoadDelegates = !(aFlags & INTERNAL_LOAD_FLAGS_DELEGATES_CHECKED); I was thinking you would have something like INTERNAL_LOAD_FLAGS_NO_DELEGATES which you would add in InternalLoadEvent::Run(), but I guess this works too. ::: mobile/android/modules/geckoview/LoadURIDelegate.jsm @@ +27,5 @@ > flags: aFlags > }; > > + return aEventDispatcher.sendRequestForResult(message).catch(() => { > + return Promise.resolve(false); I think you can just do `return false` here.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #17) > Comment on attachment 8988877 [details] [diff] [review] > Make nsILoadURIDelegate async > > Review of attachment 8988877 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: docshell/base/nsDocShell.cpp > @@ +9485,5 @@ > > > > const bool isDocumentAuxSandboxed = doc && > > (doc->GetSandboxFlags() & SANDBOXED_AUXILIARY_NAVIGATION); > > > > + const bool checkLoadDelegates = !(aFlags & INTERNAL_LOAD_FLAGS_DELEGATES_CHECKED); > > I was thinking you would have something like > INTERNAL_LOAD_FLAGS_NO_DELEGATES which you would add in > InternalLoadEvent::Run(), but I guess this works too. I think that would be safe right now, but I don't want to assume any future usage of InternalLoadEvent has gone through checking for load delegates. > ::: mobile/android/modules/geckoview/LoadURIDelegate.jsm > @@ +27,5 @@ > > flags: aFlags > > }; > > > > + return aEventDispatcher.sendRequestForResult(message).catch(() => { > > + return Promise.resolve(false); > > I think you can just do `return false` here. Yup, you're right.

Make sure this doesn't break popup blocking, as explained at [1]. If it does, I think you'd need something similar to what wedo in `OnLinkClickEvent::Run` [1] https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#13201

(In reply to (pto) Jim Chen [:jchen] [:darchons] from comment #19) > Make sure this doesn't break popup blocking, as explained at [1]. If it > does, I think you'd need something similar to what wedo in > `OnLinkClickEvent::Run` > > [1] > https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell. > cpp#13201 This doesn't appear to be an issue as far as I can tell from manual testing.

I'm sorry for the horrible lag here... Still digging out from vacation mail. :(

Comment on attachment 8988877 [details] [diff] [review] Make nsILoadURIDelegate async >+ NS_DECL_ISUPPORTS This class is holding a strong ref to the InternalLoadEvent. The event holds strong references to various stuff. Nothing says the Promise will ever settle (or that AppendNativeHandler will append anything, for that matter). Or to put another way, this class needs to be cycle-collected. And InternalLoadEvent needs to become cycle-collected as well. > rv = mLoadURIDelegate->LoadURI(aURI, where, aFlags, aTriggeringPrincipal, After this call, shouldn't "rv" be checked before trying to work with "promise", not after? If rv is failure, then the promise->AppendNativeHandler(handler) call is a crash at best. Realistically, depending on what the JS does you could have a success rv here and still a null promise.... >+ const long INTERNAL_LOAD_FLAGS_DELEGATES_CHECKED = 0x400; I wonder why bug 1439013 added 0x1000 but skipped 0x400. Maybe check with the patch author there? >+ return aEventDispatcher.sendRequestForResult(message).catch(() => { >+ return Promise.resolve(false); > }); Is there a reason this isn't the simpler return aEventDispatcher.sendRequestForResult(message).catch(() => false); ? >+++ b/xpcom/base/nsILoadURIDelegate.idl This needs to document what values the returned Promise may be resolved with and what they mean, as well as what a rejection means.

Thanks for the feedback. I'm not super familiar with the cycle collecting macros, but hopefully came up with something reasonable here. I think I've addressed all issues except the value of the flag in nsDocShell.idl -- unfortunately, Eugen (the author of the patch for bug 1439103) is on PTO for an extended period. I can't see any reason why 0x400 (or 0x800 for that matter) would be verboten from looking at his patch, but I've made a note to bring it up with him when he's back. I left 0x400 in as the value, but I can switch to 0x2000 if you'd rather play it safe.

Comment on attachment 8991051 [details] [diff] [review] Make nsILoadURIDelegate async Hmm. So for InternalLoadEvent that gives it two refcount members: one from Runnable (threadsafe) and one that NS_DECL_CYCLE_COLLECTING_ISUPPORTS adds (non-threadsafe, CC-enabled). I'm not 100% convinced whether that all will work right, and it's certainly confusing. How much pain would it be to move all the actual logic in InternalLoadEvent into a separate struct that InternalLoadEvent has as a member (directly, not as a pointer) and that your new PromiseNativeHandler will also have as a member? Then the refcounting can differ for the two classes but they can both delegate the work to the shared struct.

That seems like it should be pretty reasonable, I'll ping you if I run into any unforeseen problems with it.

[geckoview:klar:p1] because this bug is a Focus+GV blocker.

Updated per your suggestion.

Comment on attachment 8991937 [details] [diff] [review] Make nsILoadURIDelegate async >+ InternalLoadEvent(InternalLoadData aLoadData) This should take the arguments it used to take and just pass them through to the mLoadData constructor, instead of copying the big struct. >+ LoadURIDelegateHandler(InternalLoadData aLoadData) And this should take those same args. >+NS_IMPL_CYCLE_COLLECTION(LoadURIDelegateHandler) I expect this doesn't compile... In any case, this needs to actually declare to the CC the members of mLoadData that hold strong references to stuff. >+ if (promise) { Might as well check that before creating the LoadURIDelegateHandler. So: if (NS_SUCCEEDED(rv) && promise) {

Quick question: one of the members of InternalLoadData is a Maybe<nsCOMPtr> -- is there a good/idiomatic way of declaring this to the CC? At the moment all I can come up with is to store the underlying value of the Maybe (if it exists) along with a couple of booleans indicating whether the Maybe itself exists and if it satisfies isSome(), then declare the underlying value to the CC and reconstruct the Maybe when calling InternalLoad().

The most idiomatic way is to create overloads of ImplCycleCollectionTraverse and ImplCycleCollectionUnlink in Maybe.h that take a Maybe argument and do the right thing. You can see examples of how this is done for Nullable in dom/bindings/Nullable.h I didn't realize we didn't have those overloads already!

Updated per feedback

This overloads ImplCycleCollectionTraverse and ImplCycleCollectionUnlink for Maybe.

Comment on attachment 8992677 [details] [diff] [review] Add cycle collection implementations to Maybe r=me; this one should land before the other changeset, of course.

Comment on attachment 8992675 [details] [diff] [review] Make nsILoadURIDelegate async >+ nsresult Run() { Please leave curly on separate line. >+ aBaseURI) {} Curlies on next line, please. r=me. Sorry for the lag on this round, and thank you for bearing with this!

Pushed by droeh@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f7bd4f224f9a Add cycle collection implementations for Maybe. r=bz

Pushed by droeh@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a8f58cb7315 Make nsILoadURIDelegate async to preserve the order of GeckoSession.loadUri() calls. r=snorp,bz

Pushed by droeh@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c635e583eeb3 Fix eslint failure. r=me on CLOSED TREE

Dylan can you land a follow-up to re-enable the "multipleLoads" test? [1] [1] https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ProgressDelegateTest.kt#89

Backed out changeset 9a8f58cb7315 (bug 1441059) for geckoview failures on multiple files. Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba9884ab1267e546a1a2fc6c653c4d5b095329b0 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9a8f58cb7315a92f9ebede217e1b8d968c5e5a1b&selectedJob=189563360 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=189563360&repo=mozilla-inbound&lineNumber=1549 [task 2018-07-23T16:32:55.290Z] 16:32:55 INFO - TEST-START | org.mozilla.geckoview.test.AccessibilityTest.testCheckbox [task 2018-07-23T16:35:21.672Z] 16:35:21 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: id=AndroidJUnitRunner [task 2018-07-23T16:35:21.672Z] 16:35:21 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: current=11 [task 2018-07-23T16:35:21.672Z] 16:35:21 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: class=org.mozilla.geckoview.test.AccessibilityTest [task 2018-07-23T16:35:21.672Z] 16:35:21 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: stream= [task 2018-07-23T16:35:21.672Z] 16:35:21 INFO - org.mozilla.geckoview.test | Error in testCheckbox(org.mozilla.geckoview.test.AccessibilityTest): [task 2018-07-23T16:35:21.672Z] 16:35:21 INFO - org.mozilla.geckoview.test | org.mozilla.geckoview.test.util.UiThreadUtils$TimeoutException: Timed out after 120000ms [task 2018-07-23T16:35:21.672Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.util.UiThreadUtils$TimeoutRunnable.run(UiThreadUtils.java:48) [task 2018-07-23T16:35:21.672Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.os.Handler.handleCallback(Handler.java:730) [task 2018-07-23T16:35:21.672Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.os.Handler.dispatchMessage(Handler.java:92) [task 2018-07-23T16:35:21.672Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.util.UiThreadUtils.loopUntilIdle(UiThreadUtils.java:100) [task 2018-07-23T16:35:21.672Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitUntilCalled(GeckoSessionTestRule.java:1735) [task 2018-07-23T16:35:21.672Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitForPageStops(GeckoSessionTestRule.java:1540) [task 2018-07-23T16:35:21.673Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitForPageStop(GeckoSessionTestRule.java:1515) [task 2018-07-23T16:35:21.674Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitForPageStop(GeckoSessionTestRule.java:1505) [task 2018-07-23T16:35:21.675Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.AccessibilityTest.testCheckbox(AccessibilityTest.kt:386) [task 2018-07-23T16:35:21.675Z] 16:35:21 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invokeNative(Native Method) [task 2018-07-23T16:35:21.676Z] 16:35:21 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invoke(Method.java:525) [task 2018-07-23T16:35:21.676Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) [task 2018-07-23T16:35:21.677Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) [task 2018-07-23T16:35:21.678Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) [task 2018-07-23T16:35:21.678Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) [task 2018-07-23T16:35:21.679Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) [task 2018-07-23T16:35:21.679Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) [task 2018-07-23T16:35:21.680Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule$4.evaluate(GeckoSessionTestRule.java:1486) [task 2018-07-23T16:35:21.680Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.support.test.internal.statement.UiThreadStatement$1.run(UiThreadStatement.java:44) [task 2018-07-23T16:35:21.681Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.app.Instrumentation$SyncRunnable.run(Instrumentation.java:1719) [task 2018-07-23T16:35:21.681Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.os.Handler.handleCallback(Handler.java:730) [task 2018-07-23T16:35:21.682Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.os.Handler.dispatchMessage(Handler.java:92) [task 2018-07-23T16:35:21.682Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.os.Looper.loop(Looper.java:137) [task 2018-07-23T16:35:21.682Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.app.ActivityThread.main(ActivityThread.java:5103) [task 2018-07-23T16:35:21.683Z] 16:35:21 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invokeNative(Native Method) [task 2018-07-23T16:35:21.683Z] 16:35:21 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invoke(Method.java:525) [task 2018-07-23T16:35:21.684Z] 16:35:21 INFO - org.mozilla.geckoview.test | at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:737) [task 2018-07-23T16:35:21.685Z] 16:35:21 INFO - org.mozilla.geckoview.test | at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553) [task 2018-07-23T16:35:21.685Z] 16:35:21 INFO - org.mozilla.geckoview.test | at dalvik.system.NativeStart.main(Native Method) [task 2018-07-23T16:35:21.686Z] 16:35:21 INFO - org.mozilla.geckoview.test | [task 2018-07-23T16:35:21.686Z] 16:35:21 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: numtests=163 [task 2018-07-23T16:35:21.687Z] 16:35:21 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: stack=org.mozilla.geckoview.test.util.UiThreadUtils$TimeoutException: Timed out after 120000ms [task 2018-07-23T16:35:21.688Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.util.UiThreadUtils$TimeoutRunnable.run(UiThreadUtils.java:48) [task 2018-07-23T16:35:21.688Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.os.Handler.handleCallback(Handler.java:730) [task 2018-07-23T16:35:21.689Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.os.Handler.dispatchMessage(Handler.java:92) [task 2018-07-23T16:35:21.689Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.util.UiThreadUtils.loopUntilIdle(UiThreadUtils.java:100) [task 2018-07-23T16:35:21.690Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitUntilCalled(GeckoSessionTestRule.java:1735) [task 2018-07-23T16:35:21.691Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitForPageStops(GeckoSessionTestRule.java:1540) [task 2018-07-23T16:35:21.691Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitForPageStop(GeckoSessionTestRule.java:1515) [task 2018-07-23T16:35:21.692Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitForPageStop(GeckoSessionTestRule.java:1505) [task 2018-07-23T16:35:21.692Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.AccessibilityTest.testCheckbox(AccessibilityTest.kt:386) [task 2018-07-23T16:35:21.693Z] 16:35:21 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invokeNative(Native Method) [task 2018-07-23T16:35:21.694Z] 16:35:21 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invoke(Method.java:525) [task 2018-07-23T16:35:21.694Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) [task 2018-07-23T16:35:21.695Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) [task 2018-07-23T16:35:21.695Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) [task 2018-07-23T16:35:21.696Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) [task 2018-07-23T16:35:21.696Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) [task 2018-07-23T16:35:21.697Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) [task 2018-07-23T16:35:21.698Z] 16:35:21 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule$4.evaluate(GeckoSessionTestRule.java:1486) [task 2018-07-23T16:35:21.699Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.support.test.internal.statement.UiThreadStatement$1.run(UiThreadStatement.java:44) [task 2018-07-23T16:35:21.699Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.app.Instrumentation$SyncRunnable.run(Instrumentation.java:1719) [task 2018-07-23T16:35:21.699Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.os.Handler.handleCallback(Handler.java:730) [task 2018-07-23T16:35:21.700Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.os.Handler.dispatchMessage(Handler.java:92) [task 2018-07-23T16:35:21.700Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.os.Looper.loop(Looper.java:137) [task 2018-07-23T16:35:21.701Z] 16:35:21 INFO - org.mozilla.geckoview.test | at android.app.ActivityThread.main(ActivityThread.java:5103) [task 2018-07-23T16:35:21.701Z] 16:35:21 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invokeNative(Native Method) [task 2018-07-23T16:35:21.702Z] 16:35:21 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invoke(Method.java:525) [task 2018-07-23T16:35:21.703Z] 16:35:21 INFO - org.mozilla.geckoview.test | at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:737) [task 2018-07-23T16:35:21.703Z] 16:35:21 INFO - org.mozilla.geckoview.test | at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553) [task 2018-07-23T16:35:21.704Z] 16:35:21 INFO - org.mozilla.geckoview.test | at dalvik.system.NativeStart.main(Native Method) [task 2018-07-23T16:35:21.704Z] 16:35:21 INFO - org.mozilla.geckoview.test | [task 2018-07-23T16:35:21.704Z] 16:35:21 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: test=testCheckbox [task 2018-07-23T16:35:21.705Z] 16:35:21 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS_CODE: -2 [task 2018-07-23T16:35:21.705Z] 16:35:21 WARNING - TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.AccessibilityTest.testCheckbox | status -2

Forgot to backout the follow-up related to ESlint failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c635e583eeb3a483d664a37bdadb1a9c3c41f1d4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad2388a6a0aeecc74c35f5d11bfb748aa88dfa21

We decided that this bug does not need to block Focus+GV if the redirect bug doesn't block.

Pushed by droeh@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e66e6bd82e3f Make nsILoadURIDelegate async to preserve the order of GeckoSession.loadUri() calls. r=snorp,bz

status-firefox62=wontfix because, IIUC, we don't need to uplift this fix to 62 Beta.

(In reply to Jim Chen [:jchen] [:darchons] from comment #39) > Dylan can you land a follow-up to re-enable the "multipleLoads" test? [1] > > [1] > https://searchfox.org/mozilla-central/rev/ > ad36eff63e208b37bc9441b91b7cea7291d82890/mobile/android/geckoview/src/ > androidTest/java/org/mozilla/geckoview/test/ProgressDelegateTest.kt#89 I just rolled this change into the patch when I landed it.

Comment on attachment 8992675 [details] [diff] [review] Make nsILoadURIDelegate async [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for consideration: N/A User impact if declined: We won't be able to uplift the patch for bug 1478171, certain redirects may not work in GeckoView Fix Landed on Version: 63 Risk to taking this patch (and alternatives if risky): Should not impact Firefox/Fennec at all, solely GeckoView. Should be relatively low risk. String or UUID changes made by this patch: N/A See https://wiki.mozilla.org/Release_Management/Uplift_rules for more info.

Comment on attachment 8992677 [details] [diff] [review] Add cycle collection implementations to Maybe [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for consideration: N/A User impact if declined: We won't be able to land the patch for bug 1478171, some redirects may not work correctly in GeckoView apps. Fix Landed on Version: 63 Risk to taking this patch (and alternatives if risky): Very low, just adds cycle collection implementations for Maybe. String or UUID changes made by this patch: None See https://wiki.mozilla.org/Release_Management/Uplift_rules for more info.

Comment on attachment 8992675 [details] [diff] [review] Make nsILoadURIDelegate async GV change, needed for Focus release

Comment on attachment 8992677 [details] [diff] [review] Add cycle collection implementations to Maybe GV change, needed for Focus release

Fix lint error and uplift part 1 to fix bustage https://hg.mozilla.org/releases/mozilla-release/rev/76f94cb522c7e386c55491311f901a9dd96d6ec2 https://hg.mozilla.org/releases/mozilla-release/rev/427eacc1a0bfc1bd07f9c41e16d9d03a739dce7c

Backed out from GECKOVIEW_62_RELBRANCH for causing bug 1489257. https://hg.mozilla.org/releases/mozilla-release/rev/4cdeecd31350b13f8741202dc06d87d8bc3d84db

Comment on attachment 8992677 [details] [diff] [review] Add cycle collection implementations to Maybe Per IRC discussion with droeh.

Backed out in bug 1489257

Jim, do you have any suggestions on how to approach the HTMLFormElement problem we ran into?

If you look at what `HTMLFormElement` does with the docshell/request, it mostly uses them to register an `nsIWebProgressListener` [1]. To fix that, instead of having `InternalLoad` return a docshell/request, I think we want to make it accept an `nsIWebProgressListener`, so the call becomes completely async. Essentially, you want `nsDocShell` to implement the block of code at [1] using an `nsIWebProgressListener` that `HTMLFormElement` passes in. You can ask :bz or :smaug too on their thoughts.

[1] https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/dom/html/HTMLFormElement.cpp#764-783

Thanks; it's a bit more involved than that because we need to be able to call ForgetCurrentSubmission on the HTMLFormElement when necessary if we can't rely on synchronously getting a docshell outparam. I have something working (tested manually with Google logins) here, but it's a bit uglier than I'd like. (For bz: the original patch you reviewed for this had to be backed out because HTMLFormElement::SubmitSubmission() relies on synchronously getting the docshell outparam from InternalLoad, which ended up breaking all google login pages.)

Comment on attachment 9012394 [details] [diff] [review] Make nsILoadURIDelegate async Review of attachment 9012394 [details] [diff] [review]: ----------------------------------------------------------------- I was thinking you would go through `nsIWebProgressListener` for all interaction between `nsDocShell` and `HTMLFormElement`, so that `HTMLFormElement` would not see the `nsIDocShell` or `nsIRequest` at all. For example, the main reason for calling `ForgetCurrentSubmission` is because `HTMLFormElement` keeps a reference to a `nsIRequest`, but if `HTMLFormElement` doesn't see the `nsIRequest` at all, it wouldn't need `ForgetCurrentSubmission`.

(In reply to Jim Chen [:jchen] [:darchons] from comment #60) > Comment on attachment 9012394 [details] [diff] [review] > Make nsILoadURIDelegate async > > Review of attachment 9012394 [details] [diff] [review]: > ----------------------------------------------------------------- > > I was thinking you would go through `nsIWebProgressListener` for all > interaction between `nsDocShell` and `HTMLFormElement`, so that > `HTMLFormElement` would not see the `nsIDocShell` or `nsIRequest` at all. > For example, the main reason for calling `ForgetCurrentSubmission` is > because `HTMLFormElement` keeps a reference to a `nsIRequest`, but if > `HTMLFormElement` doesn't see the `nsIRequest` at all, it wouldn't need > `ForgetCurrentSubmission`. Maybe I'm just misunderstanding something, but this doesn't make sense to me at all. In my understanding: * ForgetCurrentSubmission is important for ensuring that mIsSubmitting and mNotifiedObservers are correct and removing the progress listener; freeing the reference to the nsIRequest is just a byproduct. * HTMLFormElement only keeps a reference to the nsIRequest to make sure that it can call ForgetCurrentSubmission in response to STATE_STOP for the *correct* request. Without this, the request could just be confined to SubmitSubmission() as far as I can tell. I don't see how we can ensure that mIsSubmitting and mNotifiedObservers are properly updated with what you're suggesting, and both serve important purposes.

What I meant was `nsDocShell` can always call `nsIWebProgressListener::OnStateChange` with STATE_STOP in situations where `HTMLFormElement` currently calls `ForgetCurrentSubmission`. So in effect `ForgetCurrentSubmission` is still called but indirectly.

(In reply to Jim Chen [:jchen] [:darchons] from comment #62) > What I meant was `nsDocShell` can always call > `nsIWebProgressListener::OnStateChange` with STATE_STOP in situations where > `HTMLFormElement` currently calls `ForgetCurrentSubmission`. So in effect > `ForgetCurrentSubmission` is still called but indirectly. I still don't think that's viable, though? HTMLFormElement only wants to call ForgetCurrentSubmission in response to STATE_STOP for a particular request (the one created by InternalLoad), and it seems like there may be cases where InternalLoad returns NS_OK with aRequest set to null, but we still want to immediately call ForgetCurrentSubmission -- which doesn't seem possible to do correctly via nsIWebProgressListener. Essentially, * We'd still need to store the nsIRequest in HTMLFormElement to make sure we're calling ForgetCurrentSubmission in response to STATE_STOP for the correct request * There are (I think) cases where InternalLoad succeeds but does not create a request, in which case we still need to call ForgetCurrentSubmission Let's talk on Slack on Monday, I guess.

Comment on attachment 9012394 [details] [diff] [review] Make nsILoadURIDelegate async Canceling review request until I update per jim's suggestions.

Here's a first pass at what we talked about. Some issues: First, it doesn't actually work -- behavior on google logins is the same as before. Second, I'm not clear on what the advantage of using the progress listener to communicate is; to add the HTMLFormElement as a listener we need a reference to it in InternalLoad(), at which point we might as well just call ForgetSubmission() directly -- am I missing something here?

Comment on attachment 9013695 [details] [diff] [review] Make nsILoadURIDelegate async Got feedback from Jim on slack, updating accordingly.

Alright, Jim, I think you had in mind something like this? Unfortunately, it's still not actually working -- we get the same behavior on Google login pages with or without this patch (as long as patches 1&2 are applied). I'm investigating further to try and figure out what's going on, but if you've got any ideas or you see anything I've missed in this patch please let me know.

Comment on attachment 9015351 [details] [diff] [review] 3.0 - Make onLinkClickSync async Review of attachment 9015351 [details] [diff] [review]: ----------------------------------------------------------------- Have you stepped through the code to see when `ForgetCurrentSubmission` is actually getting called? I think instead of spreading `OnStateChange` around everywhere, we should check in `OnLinkClickSync` if the load was async. If the load was not async, we call `OnStateChange` from `OnLinkClickSync` only. ::: docshell/base/nsDocShell.cpp @@ +9811,5 @@ > > // Else we ran out of memory, or were a popup and got blocked, > // or something. > + if (NS_SUCCEEDED(rv) && aWebProgressListener) { > + aWebProgressListener->OnStateChange(nullptr, nullptr, This isn't quite right, because the `InternalLoad` call above may have succeeded, in which case we shouldn't signal the state change at this point.

63=wontfix because launching app links (like Reddit's) is a low priority for Focus.

(In reply to Jim Chen [:jchen] [:darchons] from comment #70) > Comment on attachment 9015351 [details] [diff] [review] > 3.0 - Make onLinkClickSync async > > Review of attachment 9015351 [details] [diff] [review]: > ----------------------------------------------------------------- > > Have you stepped through the code to see when `ForgetCurrentSubmission` is > actually getting called? Yeah, it looks to me like we are getting only the expected ForgetCurrentSubmission call from InternalLoad/OnLinkClickSync, but I'm seeing an extraneous ForgetCurrentSubmission call coming from HTMLFormElement::UnbindFromTree compared to when I go through a Google login without these patches applied, so there may be something subtler going on here. bz, can you offer any insight here?

Sorry for the lag here; I was trying to page this in. As far as the ForgetCurrentSubmission call from UnbindFromTree, that's a bit odd. I'd would expect the site is removing the form from the DOM, or not, independently from these patches. For the overall problem, it seems to me that ideally we would align closer to the spec here. Per spec we would queue a task to do the submission navigation, then cancel that task if a new submit attempt happens and not have to keep track of any loads. That's different from our current setup where the later attempts are ignored instead of the earlier ones being dropped if there are multiple submit attempts. That said, I can't tell whether the spec would effectively suffer from bug 72906. I suspect it might, because the spec's "planned navigation" task would run before the second click happens.

Hmm, submission handling... reminds me of bug 1495363, which I need to still give feedback.

bz, I'm not sure. It also depends on how much in "navigate" is done synchronously as unloading might cause the second navigation attempt to end up ignored.