466681 - nsIWebNavigation flags are pretty broken

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We seem to have shipped 1.9 with some serious breakage in our nsIWebNavigation flags:

1)  LOAD_FLAGS_BYPASS_CLASSIFIER is outside the range of allowed flags (0xffff
    max)
2)  LOAD_FLAGS_ALLOW_POPUPS == LOAD_FLAGS_ERROR_PAGE.  We don't mask off
    LOAD_FLAGS_ALLOW_POPUPS as EXTRA_FLAGS, so anyone passing that flag will get
    a LOAD_ERROR_PAGE load.

I think we should probably move LOAD_FLAGS_ERROR_PAGE into the IDL so that there are no more collisions with it, mask out LOAD_FLAGS_ALLOW_POPUPS via EXTRA_FLAGS, change the value of LOAD_FLAGS_BYPASS_CLASSIFIER to the free value at 0x0001.

Alternately, we could allow flags > 0xffff but only if they're always masked off via EXTRA_FLAGS, and leave BYPASS_CLASSIFIER as-is and document things better.  This has the benefit of not changing existing flag values (other than LOAD_FLAGS_ERROR_PAGE, which could move into 0x0001 or something).

Thoughts?  We probably want to do this for 1.9.1, since the resolution of this is sorta blocking what we do with flags in bug 437174.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #0)
> 1)  LOAD_FLAGS_BYPASS_CLASSIFIER is outside the range of allowed flags (0xffff
>     max)

This is filed as bug 435297, FWIW.

Comment 2

[Johnny Stenback (:jst)]

Boris, your proposal sounds good to me, I can't think of anything better given the mess we're in.

Comment 3

[Johnny Stenback (:jst)]

And yes, we should fix this for 1.9.1 to stop the mess from spreading further than it has to.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so should work, I think

Most of this is straightforward, but double-check the 0xf flags being unused, please?  I couldn't find a place where they're used...

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 350034 [details] [diff] [review]
Like so should work, I think

Looks good to me.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozlla.org/mozilla-central/rev/a9c8796253e9

Comment 7

[Boris Zbarsky [:bzbarsky]]

Make that http://hg.mozilla.org/mozilla-central/rev/a9c8796253e9

Comment 8

[Reed Loden [:reed]]

Comment on attachment 350034 [details] [diff] [review]
Like so should work, I think

>+ * Load flag for error pages. This uses one of the reserved flag
>+ * values from nsIWebNabvigation.

Typo.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

+    NS_PRECONDITION((aLoadFlags & 0xf) == 0, "Should have these flags set");

Should -> Should not?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed both of those issues.

Comment 11

[Aakash Desai [:aakashd]]

At the risk of sounding dumb, is there a specific testcase or set of testcases that would be able to verify something like this?

Comment 12

[Boris Zbarsky [:bzbarsky]]

You could write some code that does a load with LOAD_FLAGS_ALLOW_POPUPS and check for broken resulting behavior (e.g. lack of progress events), I think.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Aakash, not that I know of. And after reading this bug it sounds more like we should have an automated test for that.