Closed Bug 1081879 Opened 5 years ago Closed 5 years ago

[e10s] Can not get nsIHttpChannel in nsIWebProgressListener.onStateChange

Categories

(Core :: General, defect)

33 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
e10s m6+ ---
firefox40 --- fixed

People

(Reporter: pzhang, Assigned: billm)

References

Details

Attachments

(1 file)

We can't get nsIHttpChannel in the onStateChange callback of a web progress listener, considering the following codes:

  var myListener = {
    QueryInterface: XPCOMUtils.generateQI(["nsIWebProgressListener",
                                           "nsISupportsWeakReference"]),

    onStateChange: function(aWebProgress, aRequest, aFlag, aStatus) {
        // |aRequest instanceof Ci.nsIHttpChannel| is evaluated as true in *non-e10s* Nightly
        // but false in *e10s* Nightly.
        if (aRequest instanceof Ci.nsIHttpChannel) {
          alert('Got httpChannel in onStateChange: ' + aRequest.URI.spec);
        }
    },

    onLocationChange: function(aProgress, aRequest, aURI) {},
    // For definitions of the remaining functions see related documentation
    onProgressChange: function(aWebProgress, aRequest, curSelf, maxSelf, curTot, maxTot) {},
    onStatusChange: function(aWebProgress, aRequest, aStatus, aMessage) {},
    onSecurityChange: function(aWebProgress, aRequest, aState) {}
  }

  gBrowser.addProgressListener(myListener);


on *non-e10s* Nightly, |aRequest instanceof Ci.nsIHttpChannel| is  evaluates as |true|, but on *non-e10s* Nightly, it's evaluated as |false|.
Which process does the web progress listener live in, chrome or content?
Flags: needinfo?(pzhang)
(In reply to Josh Matthews [:jdm] from comment #1)
> Which process does the web progress listener live in, chrome or content?

Hi Josh, it's in the chrome process. When e10s is enabled, |aRequest| is an instance of |Ci.nsIChannel|, and i tried to |QueryInterface| Ci.nsIHttpChannel on |aRequest|, it failed.
Flags: needinfo?(pzhang) → needinfo?(josh)
Assignee: nobody → wmccloskey
Sorry for letting this sit; Bill appears to be on the case, now.
Flags: needinfo?(josh)
Attached patch patchSplinter Review
This patch adds shims around gBrowser.addProgressListener and addTabsProgressListener. Add-on listeners will be passed CPOWs for the nsIWebProgress and nsIRequest arguments. (Right now we just pass in fake objects that are close enough to fool our frontend code.)

Additionally, listeners are run synchronously. That is, the content process blocks waiting for them to finish. That way add-ons have a chance to examine the state of the content process using CPOWs.
Attachment #8589312 - Flags: review?(mconley)
Duplicate of this bug: 1118880
Comment on attachment 8589312 [details] [diff] [review]
patch

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

So this looks OK, and should, as far as I can tell, give old shimmed add-ons what they need. I think it's particularly clever how you expose the nsIRequest CPOWs _just_ for add-ons!

::: toolkit/components/addoncompat/RemoteAddonsChild.jsm
@@ +547,5 @@
>        }
>      }
>    },
> +
> +  useSyncWebProgress() {

Nit - might as well use a getter here, since you don't take arguments.

::: toolkit/components/addoncompat/tests/addon/bootstrap.js
@@ +552,5 @@
>      then(testObserver).
>      then(testSandbox).
>      then(testAddonContent).
> +    then(testAboutModuleRegistration).
> +    then(testProgressListener);

Just a thought - if we wanted to not taint the blame of the last entry every time we add a new one, we could take this opportunity to put this at the end:

.then(testProgressListener)
.then(Promise.resolve());

If you want - just something I thought of.
Attachment #8589312 - Flags: review?(mconley) → review+
I tried this patch, the interface `Ci.nsIHttpChannel` could be queried on the parameter `aRequest` now, thanks.

But error occured when visiting headers on the nsIHttpChannel object:
  [object CPOW [Exception... "It's illegal to pass a CPOW to native code arg 0 [nsIHttpChannel.visitRequestHeaders]"  nsresult: "0x80570036 (NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE)"  location: "JS frame :: chrome://global/content/browser-child.js :: _send :: line 105"  data: no]]

and here is the code:
  gBrowser.addTabsProgressListener({
    onStateChange: function(aBrowser, aWebProgress, aRequest, aStateFlags, aStatus) {
      if (!(aStateFlags & (Ci.nsIWebProgressListener.STATE_TRANSFERRING | Ci.nsIWebProgressListener.STATE_IS_DOCUMENT))) {
        return;
      }

      let httpChannel = aRequest.QueryInterface(Ci.nsIHttpChannel);
      httpChannel.visitRequestHeaders({
        visitHeader: function(aHeader, aValue) {
          console.log(aHeader + ': ' + aValue);
        }
      });
    }
  });
Depends on: 1160217
https://hg.mozilla.org/mozilla-central/rev/e158ed184326
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Blocks: 1160910
A quick heads up that the tabbrowser addProgressListener() still produces a request that cannot be QueryInterface'd to a channel (with e10s enabled and the addon declared multiprocess-compatible).

I hesitate to even mention it because I don't think we should provide extra shims or synchronous messaging in this case.  The tabbrowser progress listener behaviour cannot easily be replaced by a frame script and I think it should remain available even to multiprocess-compatible code without any baggage.  Possibly the request parameter should even be made null in that case since it isn't reliable.

Note that the web progress DOMWindow property is still shimmed even for addons marked compatible, I think intentionally, but this object may become invalid when accessed asynchronously from the tabbrowser listener.  Bug 1118880 describes this situation.  Again, I don't think it should be changed, but either documented or possible set null.
You need to log in before you can comment on or make changes to this bug.