Closed Bug 582477 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Networking: HTTP, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b1+ ---

People

(Reporter: cjones, Assigned: cjones)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Assignee: nobody → jones.chris.g
Attachment #460753 - Flags: review?(jduell.mcbugs)
If bug 540097 gets blocking+, this should as well, since we can't in good conscience turn on strong checking without this bug fixed.
Blocks: 540097
tracking-fennec: --- → ?
(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.
(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.
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 536317
IMHO this is a totally bogus dup.  Please let's land Cancel() soon, in the passive-aggressive sense.
Reopening, marking as depending on Cancel.  Is that better?  At least this way we won't forget to test.
Blocks: fennecko
Status: RESOLVED → REOPENED
Depends on: 536317
Resolution: DUPLICATE → ---
No, but we have bigger fish to fry, so let's not worry about it anymore.  Onward ho!
tracking-fennec: ? → 2.0b1+
Duplicate of this bug: 584149
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.
Will give this another shot.  In the interim, I found another |return false;| that this v2 fixes.
Attachment #460753 - Attachment is obsolete: true
Attachment #462828 - Flags: review?(jduell.mcbugs)
Attachment #460753 - Flags: review?(jduell.mcbugs)
http://hg.mozilla.org/mozilla-central/rev/f83272c77b8f
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
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
Attachment #462828 - Flags: review?(jduell.mcbugs) → review+
You need to log in before you can comment on or make changes to this bug.