808389 - nsIProtocolHandler uses same values for different flags

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

const unsigned long URI_SYNC_LOAD_IS_OK = (1<<15);
const unsigned long URI_FORBIDS_COOKIE_ACCESS = (1<<15);

This looks really odd.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Bug 557047 is also related since it added URI_FORBIDS_COOKIE_ACCESS, but that was before 
URI_SYNC_LOAD_IS_OK.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Ouch.  Yeah, that's broken, as is the fact that the flags are not in order on the interface.

We should switch URI_SYNC_LOAD_IS_OK to an empty bit.  Want to do it?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Yeah, I can do it.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: simple

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: alternative,

This makes the flags to be in order and also needs to switch the values
of URI_LOADABLE_BY_SUBSUMERS and URI_NON_PERSISTABLE.

Review the one you prefer. I'd take this patch.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

simple: https://tbpl.mozilla.org/?tree=Try&rev=193d1d328f09
alternative: https://tbpl.mozilla.org/?tree=Try&rev=0fc5471004d7

Comment 7

[Masatoshi Kimura [:emk]]

I think you need to bump an iid for either patches because changing constatnt values will break binary compatibility.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

indeed.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: simple

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: alternative

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 678168 [details] [diff] [review]
alternative

I think I like this one.  r=me, but should get sr from biesi.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 678167 [details] [diff] [review]
simple

This is fine by me too, would need an sr as well.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/50cd5f7339f6

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Masatoshi Kimura [:emk] from comment #7)
> I think you need to bump an iid for either patches because changing
> constatnt values will break binary compatibility.

Actually, changing constants doesn't break binary compatibility.

Comment 15

[Masatoshi Kimura [:emk]]

(In reply to :Ms2ger from comment #14)
> (In reply to Masatoshi Kimura [:emk] from comment #7)
> > I think you need to bump an iid for either patches because changing
> > constatnt values will break binary compatibility.
> 
> Actually, changing constants doesn't break binary compatibility.

If a binary component had used the constant URI_NON_PERSISTABLE in the source code, only the value (1<<10) would be embeded in the binary. Now, Gecko expects (1<<14) as a value for URI_NON_PERSISTABLE. JavaScript components would have no trouble because they usually refer the constant using the symbol.
Doesn't it break binary compatibility?

Comment 16

[Boris Zbarsky [:bzbarsky]]

_Adding_ constants does not change binary compat.

Changing the meaning of existing numeric values of course breaks binary compat...