582477 - [e10s] HttpChannelChild triggers abort on bing.com (after bug 540097)

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

In HttpChannelChild.cpp, we're hitting the |return false;| here

  rv = mListener->OnDataAvailable(this, mListenerContext,
                                  stringStream, offset, count);
  stringStream->Close();
  if (NS_FAILED(rv)) {
    // TODO: Cancel request: see OnStartRequest. Bug 536317
    return false; 
  }

This is caused by

    rv = mImage->Init(this, mContentType.get(), containerFlags);
    if (NS_FAILED(rv)) { // Probably bad mimetype

      // There's no reason to keep the image around. Save memory.
      //
      // XXXbholley - This is also here because I'm not sure we've found
      // all the consumers who (incorrectly) check whether the container
      // is null to determine things like size availability (they should
      // be checking the image status instead).
      mImage = nsnull;

      this->Cancel(rv);
      return NS_BINDING_ABORTED;
    }

in imgRequest.cpp.  It apparently doesn't like

(gdb) p mContentType
$2 = {
  <nsACString_internal> = {
    mData = 0x28a91e8 "application/json", 
    mLength = 16, 
    mFlags = 5
  }, <No data fields>}

Can't say I blame it.  bing.com looks to be at fault.  However, HttpChannelChild shouldn't be returning false here; this isn't a protocol/transport error, it's just malformed web content.  We have to deal with that gracefully.  I think we should be returning true in this case.

Comment 1

[Josh Matthews [:jdm]]

I would agree, but I don't know if it's really worth making this change versus waiting for the Cancel work to finish up and land (bug 536317).  Although I guess now that the child process aborts on any failure, this might have a larger impact.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: HTTP channel listener errors shouldn't be reported as IPC protocol/transport errors

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

If bug 540097 gets blocking+, this should as well, since we can't in good conscience turn on strong checking without this bug fixed.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #1)
> I would agree, but I don't know if it's really worth making this change versus
> waiting for the Cancel work to finish up and land (bug 536317).  Although I
> guess now that the child process aborts on any failure, this might have a
> larger impact.

This is totally orthogonal to Cancel being implemented, unless those patches incidentally change this return value.

Comment 5

[Josh Matthews [:jdm]]

(In reply to comment #4)
> This is totally orthogonal to Cancel being implemented, unless those patches
> incidentally change this return value.

Which in fact they do.

Comment 6

[Jason Duell]

I'd rather land Cancel and fix these in that patch.  Reopen if there winds up being a time constraint--I agree we should fix this in anything we ship.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

IMHO this is a totally bogus dup.  Please let's land Cancel() soon, in the passive-aggressive sense.

Comment 8

[Jason Duell]

Reopening, marking as depending on Cancel.  Is that better?  At least this way we won't forget to test.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

No, but we have bigger fish to fry, so let's not worry about it anymore.  Onward ho!

Comment 11

[Josh Matthews [:jdm]]

I'm all for landing this, as Cancel doesn't seem in any danger of being finished up imminently.  These errors are annoying and quite common on some pages.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: HTTP channel listener errors shouldn't be reported as IPC protocol/transport errors, v2

Will give this another shot.  In the interim, I found another |return false;| that this v2 fixes.

Comment 13

[Jason Duell]

http://hg.mozilla.org/mozilla-central/rev/f83272c77b8f

Comment 14

[Jason Duell]

Comment on attachment 462828 [details] [diff] [review]
HTTP channel listener errors shouldn't be reported as IPC protocol/transport errors, v2

forgot to mark +r