1116867 - nsIProgressEventSink not javascript compatible

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Andrew McCreight [:mccr8]]

Attachment: assertion stack

1. Make sure e10s is enabled.
2. Open the browser console.
3. Navigate to www.google.com in the main browser window.

I tried this same thing in my regular Nightly session, and nothing obviously bad happened, so maybe it is just an overly strict assertion.

Comment 1

[Josh Matthews [:jdm]]

Could you check what the actual status value is that's asserting?

Comment 2

[Patrick McManus [:mcmanus]]

so this is wfm with a debug build from this morning's m-c on linux64 - using e10s and both http:// and https:// versions of google.

is this still an issue? can you provide the info from comment 1?

Comment 3

[Andrew McCreight [:mccr8]]

Now I get this instead:

Assertion failure: progress <= progressMax (unexpected progress values), at /Users/amccreight/mc/netwerk/protocol/http/HttpChannelChild.cpp:544
#01: mozilla::net::HttpChannelChild::RecvOnTransportAndData(tag_nsresult const&, tag_nsresult const&, unsigned long long const&, unsigned long long const&, nsCString const&, unsigned long long const&, unsigned int const&)[/Users/amccreight/mc/obj-dbg.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x314f03]
#02: mozilla::net::PHttpChannelChild::OnMessageReceived(IPC::Message const&)[/Users/amccreight/mc/obj-dbg.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x64e528]
#03: mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&)[/Users/amccreight/mc/obj-dbg.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x577d87]
#04: mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&)[/Users/amccreight/mc/obj-dbg.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x46bb20]
#05: mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&)[/Users/amccreight/mc/obj-dbg.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x46a811]
#06: mozilla::ipc::MessageChannel::OnMaybeDequeueOne()[/Users/amccreight/mc/obj-dbg.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x46583f]

Comment 4

[Andrew McCreight [:mccr8]]

The values of those variables are progress = 14827 and progressMax = 0, at least through two tries of it.

Comment 5

[Andrew McCreight [:mccr8]]

Same thing happens with techcrunch.com, cnn.com, and a few other random sites I tried loading.

Comment 6

[Andrew McCreight [:mccr8]]

Though the values were different for "progress".

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Patrick, what's the significance of this bug? Should we be tracking it for e10s?

Comment 8

[Patrick McManus [:mcmanus]]

jason is probably in a better place with the ipdl code to answer the question.. valentin has a repro in 1117442 that he's working.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I am not able to reproduce any issues (comment 0 or comment 3) with the linux 64 nightly debug build (2015-01-05).  I tested in e10s mode on the same sites mentioned.

It is likely a related fix landed as part of bug 1115976, but this landed on 2015-01-03, which confuses me, since comment 3 is from 2015-01-05.

Andrew, do you still see this even in the latest debug nightly?

Comment 10

[Andrew McCreight [:mccr8]]

I downloaded firefox-37.0a1.en-US.debug-mac64.dmg from here:
  ftp://ftp.mozilla.org/pub/firefox/nightly/2015-01-06-mozilla-central-debug/
(there's no OSX nightly for 1-7 yet for some reason)
and I can reproduce it, in a fresh profile, with e10s enabled.

My steps to reproduce:
1. Hit shift-command-J to open the Browser Console.
2. Click over to the address bar in the main window.
3. Type "google.com" and hit enter.

Comment 11

[Patrick McManus [:mcmanus]]

#6  mozilla::net::HttpChannelChild::DoOnProgress (this=0x7fffdb8de000, aRequest=0x7fffdb8de088, progress=30275, progressMax=0) at ../../../../gecko-dev/netwerk/protocol/http/HttpChannelChild.cpp:544

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Okay, I am able to reproduce this now.

What happens is that the Network Monitor code (which is used when the Browser Console is open) intercepts onProgress for its own purposes, and then forwards the message along to the original onProgress listener.  Usually, this works correctly.

However, if |progressMax| is 0xFFFFFFFFFFFFFFFF (the "unknown" value), things get messed up since we are now going C++ -> JS -> C++, and JS can't safely represent integers that large, so it ends up getting forced to 0 when leaving JS.

I tried clamping the value down to Number.MAX_SAFE_INTEGER, and this does fix the assertion and crash.  However, I see that are some C++ cases where we check for exactly UINT64_MAX.

Is there some secret way I can force the JS -> C++ XPCOM conversion to allow JS to pass UINT64_MAX safely?

Comment 13

[Patrick McManus [:mcmanus]]

thanks jryans.

So nsIProgressEventSink is a broken interface, at least for JS.

I think we should update it to use a new constant nsIProgressEventSink::MAX_PROGRESS_UNKNOWN that is set to 2^53 - 1 and change each of the ~10 C++ implementations to use that instead of UINT64_MAX

That ought to fix the problem

Comment 14

[Patrick McManus [:mcmanus]]

this isn't really an e10s problem.. we see it in e10s because of an assert in the message marshalling code for onProgress().. but the root of it is that when OnProgress, which is a scriptable interface, goes through js a sentinel value is out of range of MAX_SAFE_INTEGER.. so it can't really use the interface correctly.

Comment 16

[Patrick McManus [:mcmanus]]

I'm going to put this at the top of my queue, but it isn't going to happen before the branch to ff38 next week. However the problem is long standing. We might want to consider working around any new code on 37 that triggers the issue.

(or is it not new code? and we only notice it because people have e10s enabled on nightly?)

Comment 17

[Andrew McCreight [:mccr8]]

I've been using e10s for a number of months, and I only noticed this particular problem recently.  (I work around it by not opening the console until after the page opens, which isn't too bad because I have to not open the console after the page closes due to another unrelated bug...)

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Patrick McManus [:mcmanus] from comment #16)
> I'm going to put this at the top of my queue, but it isn't going to happen
> before the branch to ff38 next week. However the problem is long standing.
> We might want to consider working around any new code on 37 that triggers
> the issue.

In bug 731318, we added code to net monitor which triggers this issue if the Browser Console or DevTools toolbox is open.  This landed in 37.

> (or is it not new code? and we only notice it because people have e10s
> enabled on nightly?)

It's a crash on e10s because HttpChannelChild uses MOZ_ASSERT[1], which forces a crash.  The same problem happens without e10s, but it's only an NS_WARNING[2].

[1]: https://hg.mozilla.org/mozilla-central/annotate/b3f84cf78dc2/netwerk/protocol/http/HttpChannelChild.cpp#l544
[2]: https://hg.mozilla.org/mozilla-central/annotate/b3f84cf78dc2/netwerk/protocol/http/nsHttpChannel.cpp#l5799

Comment 19

[Patrick McManus [:mcmanus]]

cool. so 731318 exposes a latent bug.

we need to choose between backing that out of 37, or backporting the fix of the long time bug (which is going to have to include a breaking idl change) onto aurora.

what do you think?

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Patrick McManus [:mcmanus] from comment #19)
> cool. so 731318 exposes a latent bug.
> 
> we need to choose between backing that out of 37, or backporting the fix of
> the long time bug (which is going to have to include a breaking idl change)
> onto aurora.
> 
> what do you think?

I was hoping to construct a safe way to disable for e10s only, but since the merge is on Monday, it's probably best to back out bug 731318.

I'll do this now.

Comment 21

[Patrick McManus [:mcmanus]]

thanks jryans.

Actually, this isn't really an e10s thing. only e10s throws the assert, so we notice it more there, but the single process code path is also going to corrupt maxProgress when it goes through js boundaries - it will just happen silently. The code in places like docshell that checks uint_max64 will do the wrong thing in those cases.

Comment 22

[Patrick McManus [:mcmanus]]

Attachment: make nsIProgressEventSink and nsITransportEventSink safely scriptable

These scriptable interfaces use uint_64 arguments with sentinel values
of UINT64_MAX. However, UINT64_MAX exceeds MAX_SAFE_NUMBER and cannot
be gatewayed to/from javascript - so they cannot be used
correctly. Change them to use signed 64 bit numbers and -1 as the
sentinnel. C++ implementations ought to be enough to audit as the
special value could never be used correctly in JS anyhow - also
audited OnProgressChange() uses for downstream use of this data.

Comment 23

[Patrick McManus [:mcmanus]]

Comment on attachment 8547685 [details] [diff] [review]
make nsIProgressEventSink and nsITransportEventSink safely scriptable

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

honza, I'll have bz r the xhr bits after the necko side gets squared away.

Comment 24

[Patrick McManus [:mcmanus]]

Attachment: MozReview Request: bz://1116867/mcmanus

Comment 25

[Patrick McManus [:mcmanus]]

/r/2361 - bug 1116867 - make nsIProgressEventSink and nsITransportEventSink safely scriptable r=mayhemer

Pull down this commit:

hg pull review -r 24652fa8a6703951d3b8e91fcbc03b69a7c86263

Comment 26

[Patrick McManus [:mcmanus]]

I put this in reviewboard too.. maybe you want to experiment with using that. nick and I have been using it successfully for larger patches. Doesn't matter to me which way we go.

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Patrick McManus [:mcmanus] from comment #22)
> These scriptable interfaces use uint_64 arguments with sentinel values
> of UINT64_MAX. However, UINT64_MAX exceeds MAX_SAFE_NUMBER and cannot
> be gatewayed to/from javascript

JS uses the term MAX_SAFE_INTEGER not MAX_SAFE_NUMBER, probably best to use the same terminology.

Comment 28

[Patrick McManus [:mcmanus]]

/r/2361 - bug 1116867 - make nsIProgressEventSink and nsITransportEventSink safely scriptable r=mayhemer

Pull down this commit:

hg pull review -r 8c4c1a72203118935fa4d4637229b1b1bbfbdbda

Comment 29

[Patrick McManus [:mcmanus]]

Attachment: make nsIProgressEventSink and nsITransportEventSink safely scriptable

These scriptable interfaces use uint_64 arguments with sentinel values
of UINT64_MAX. However, UINT64_MAX exceeds MAX_SAFE_NUMBER and cannot
be gatewayed to/from javascript - so they cannot be used
correctly. Change them to use signed 64 bit numbers and -1 as the
sentinnel. C++ implementations ought to be enough to audit as the
special value could never be used correctly in JS anyhow - also
audited OnProgressChange() uses for downstream use of this data.

Comment 30

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f68fc12ece7

(you can see in the diff on the try run that I changed a printf specifier in one of the C++ tests.. that's the only change)

Comment 31

[Honza Bambas (:mayhemer)]

https://reviewboard.mozilla.org/r/2361/#review1699

::: dom/base/nsXMLHttpRequest.cpp
(Diff revision 2)
>        loaded -= headerSize;

We can well get to loaded < 0 here, but out of scope of this bug.

::: embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp
(Diff revision 2)
>                uint64_t(aProgressMax), mTotalCurrentProgress, mTotalMaxProgress);

Here are some weird casts left..

::: netwerk/protocol/http/nsHttpPipeline.h
(Diff revision 2)
> -    uint64_t  mSendingToProgress;
> +    int64_t  mSendingToProgress;

indent

::: netwerk/test/TestPageLoad.cpp
(Diff revision 2)
> -    // PR_snprintf(buf, sizeof(buf), "%llu/%llu\n", progress, progressMax);
> +    // PR_snprintf(buf, sizeof(buf), "%ld/%ld\n", progress, progressMax);

lld ?

::: uriloader/base/nsDocLoader.cpp
(Diff revision 2)
>      progressDelta = int64_t(aProgress) - oldCurrentProgress;

no need to cast

::: dom/base/nsXMLHttpRequest.cpp
(Diff revision 2)
> +      (size_u64 <= 9007199254740991ULL) ? static_cast<int64_t>(size_u64) : -1;

Isn't there a define?  if not, can you please create one in some "net.h" or so?

::: embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp
(Diff revision 2)
>          mEventSink->OnProgress(request, ctxt, aProgress, aProgressMax);

And this still uses unsigned

::: netwerk/protocol/http/HttpChannelChild.cpp
(Diff revision 2)
> -      MOZ_ASSERT(progress <= progressMax, "unexpected progress values");
> +      MOZ_ASSERT((progressMax == -1) || (progress <= progressMax),

If you find some way to split this assert so that we know the progressMax value, I'd love that

::: netwerk/protocol/http/HttpChannelChild.cpp
(Diff revision 2)
> -    uint64_t progressMax(uint64_t(mOwner->GetResponseHead()->ContentLength()));
> +    int64_t progress = aOffset + uint64_t(aCount);

aCount's already uint64

Comment 32

[Honza Bambas (:mayhemer)]

https://reviewboard.mozilla.org/r/2361/#review1705

Ship It!

Comment 33

[Honza Bambas (:mayhemer)]

Patrick, is there any difference between the board patch and the regular bz patch?

Comment 34

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #33)
> Patrick, is there any difference between the board patch and the regular bz
> patch?

nope. I'll clear the regular bz one.

Comment 35

[Patrick McManus [:mcmanus]]

https://reviewboard.mozilla.org/r/2361/#review1733

> And this still uses unsigned

I can't figure this comment out.. OnProgress is changed by this patch to take unsigned

Comment 36

[Patrick McManus [:mcmanus]]

https://reviewboard.mozilla.org/r/2359/#review1735

bz can you look at (at least) the xhr part?

Comment 37

[Patrick McManus [:mcmanus]]

Comment on attachment 8547689 [details]
MozReview Request: bz://1116867/mcmanus

honza, I wasn't sure if you meant to r+ the last version (cause it said ship it) or cancel it or what.. nick had some problem with reviewboard setting flags earlier today adding to the confusion.

So I'm leaving r? set for the new rev. Actually it lets you see the interdiff which is the one place reviewboard does way better than spliter imo

Comment 38

[Boris Zbarsky [:bzbarsky]]

https://reviewboard.mozilla.org/r/2359/#review1753

So I was worried about binary extensions, but the good news is that testing an int64_t for equality against UINT64_MAX is the same as testing it for equality against -1, both in terms of machine code and C/C++ integer promotion behavior.  So there should be pretty minimal fallout, if any.

::: dom/base/nsXMLHttpRequest.cpp
(Diff revision 2)
> +      (size_u64 <= 9007199254740991ULL) ? static_cast<int64_t>(size_u64) : -1;

I would prefer we had an actual constant for MAX_SAFE_JS_INTEGER or JS_MAX_SAFE_INTEGER or whatever somewhere, either in jsapi headers (probably preferred) or at least somewhere in mfbt or something.

Then we could also static_assert things like the value being < INT64_MAX.

::: netwerk/base/public/nsITransport.idl
(Diff revision 2)
> -     *        unknown, 0xFFFFFFFF will be passed.
> +     *        unknown, -1 will be passed. tbd max_safe

The "tbd" comment here should go away, right?

::: netwerk/protocol/file/nsFileChannel.cpp
(Diff revision 2)
> +      if (avail <= 9007199254740991ULL) {

Yeah, this is why I'd prefer a named constant.

::: netwerk/protocol/ftp/nsFtpConnectionThread.cpp
(Diff revision 2)
> -    , mFileSize(UINT64_MAX)
> +    , mFileSize(9007199254740991ULL) // MAX_SAFE_NUMBER

Named constant.

::: netwerk/protocol/http/nsHttpTransaction.cpp
(Diff revision 2)
> +    mRequestSize = (size_u64 <= 9007199254740991ULL) ? static_cast<int64_t>(size_u64) : -1;

Named constant.

r=me with the nits fixed.

Comment 39

[Patrick McManus [:mcmanus]]

/r/2361 - bug 1116867 - make nsIProgressEventSink and nsITransportEventSink safely scriptable r=mayhemer r=bz

Pull down this commit:

hg pull review -r 7238b3125f1f3c327465592f7f096903928aaf18

Comment 40

[Honza Bambas (:mayhemer)]

Comment on attachment 8547689 [details]
MozReview Request: bz://1116867/mcmanus

I got lost between the patches.  But this one seems to be the r+comments I've already made before (comment 31).

Comment 41

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91e1a0afee55

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/91e1a0afee55

Comment 44

[Patrick McManus [:mcmanus]]

Attachment: MozReview Request: bug 1116867 - make nsIProgressEventSink and nsITransportEventSink safely scriptable r=mayhemer r=bz