846629 - Make nsIRequest loadFlags 64 bit.

Status: NEW

(Core :: Networking, defect, P5)

Description

[Jason Duell]

i.e. change nsIRequest.idl:

  typedef unsigned long nsLoadFlags;

to 

  typedef unsigned long long nsLoadFlags;

Reason: we'll need more flags for bug 836602 (and apparently bug 646686) and we're out of 32 bit ones (except for <<24).

This will touch a lot of code, so I'd like to have it be its own bug so we can land it quickly and avoid bitrot. Also that will let us uplift to B2G and other branches if we want (Might be a good idea: otherwise many uplifted patches will break pointlessly on long->long long context diffs).

Note that this will actually only give us 53 bits to play with, not 64, because of the vagaries of how JS handles numbers.

Ehsan, you've done large scale s/// stuff like this in the tree before.  What's the state of the art here--do we have tools that can help with this?  Also if you know of anyone who might want to take this work...  :)

Comment 1

[(no longer active)]

Your tool is sed.  :-)

Bug 690892 has a sample script for conversion.  Note that the script assumes GNU sed, so beware of running that on Mac with BSD sed (run sed --version to make sure.)

There's also a script there for unbitrotting people's mq patches, you should attach one to this bug and post to dev.platform if you expect this to bitrot a ton of stuff (but I don't think that would be needed here.)

Comment 2

[Bernhard Redl]

converting long to long long causes IDL problems

xpcom/idl-parser/xpidl.py:671
raise IDLError("const may only be a short or long type, not %s" % self.type, self.location)

is triggered while building if i change the 
-    const unsigned long LOAD_ANONYMOUS = 1 << 14;
+    const unsigned long long LOAD_ANONYMOUS = 1 << 14;

to fix this i had to change:
xpcom/idl-parser/xpidl.py:128
     Builtin('unsigned long', 'uint32_t', False, True),
-    Builtin('unsigned long long', 'uint64_t', False, False),
+    Builtin('unsigned long long', 'uint64_t', False, True),

(last param means "maybeConst") Hope this is the correct fix?

Comment 3

[Bernhard Redl]

Attachment: Change nsLoadFlags to long long + idl changes

this patch seems to break LOAD_ANONYMOUS, can anybody tell me why? What part did i miss?

Comment 4

[Bernhard Redl]

found the problem:
in resources.js
Ci.nsIRequest.LOAD_ANONYMOUS is always 0

for example 
channel.loadFlags |= Ci.nsIRequest.LOAD_ANONYMOUS
results in the call HttpBaseChannel::SetLoadFlags [loadflags=0]

channel.loadFlags = 33 results in the correct call to SetLoadFlags

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Ci.nsIRequest.LOAD_ANONYMOUS is always 0

Is is possible that something along the way converts 64-bit ints to 32-bit ones by dropping the low bits somehow?  My suspicion would fall on the code the writes or reads xpt files....

Kyle, do you know anything about that stuff?

Comment 6

[Boris Zbarsky [:bzbarsky]]

And in particular, I assume the checks disallowing 64-bit constants were there for a reason: _something_ can't deal....  :(

Comment 7

[(not currently active) Ted Mielczarek]

I suspect this is just an oversight on my part when writing the Python XPT implementation:
http://mxr.mozilla.org/mozilla-central/source/xpcom/typelib/xpt/tools/xpt.py#771

I was writing against the spec, and only fixing discrepancies by testing against our in-tree interfaces. It's likely that nothing was using 64-bit constants until now. It should be fairly easy to make that work, just extend the list of types there.

Note that I have no idea how these reflect to JS, I'd guess "okay as long as you don't use too many bits".

Comment 8

[(not currently active) Ted Mielczarek]

Reading typelib.py I don't see anything else that ought to be broken in using 64-bit constants.

Comment 9

[Bernhard Redl]

Ted, i have already included changes to xpt.py [0] and i have linked the specification i used ( http://docs.python.org/2/library/struct.html)

Hope i did this the right way.

[0] https://bugzilla.mozilla.org/attachment.cgi?id=728257&action=diff#a/xpcom/typelib/xpt/tools/xpt.py_sec2

maybe unrelated but i also found nsVariant.cpp:87
    case nsIDataType::VTYPE_INT64:
    case nsIDataType::VTYPE_UINT64:
        // XXX Need boundary checking here.
        // We may need to return NS_SUCCESS_LOSS_OF_INSIGNIFICANT_DATA
        outData->u.mDoubleValue = double(inData.u.mInt64Value);
        outData->mType = nsIDataType::VTYPE_DOUBLE;
        return NS_OK;

long long seems to be represented as double, maybe a problem for checking if a bit is set?

Comment 10

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258