916604 - DocShell assertion when testing app redirects

Status: RESOLVED INVALID

(DevTools Graveyard :: WebIDE, defect, P3)

Description

[Alexandre Poirot [:ochameau]]

I've been writing a mochitest to cover app redirect for multiple weeks now, and this test used to pass but has started triggering an assertion recently:
mLoadType != LOAD_ERROR_PAGE && mLoadType != LOAD_BYPASS_HISTORY (Do not add error or bypass pages to global history)
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#11461

When creating an mozapp iframe whose app has a special redirects rule in its manifest, it triggers this exception when loading a document that is redirected and whose redirection is catched by the app redirect rule and is supposed to be redirected to a local app document.

See test script in attachment.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Add test for redirects in webapps actor mochitest r=fabrice

Comment 2

[Alexandre Poirot [:ochameau]]

Fabrice, you wrote the original code in docshell.cpp for app redirects, does that assertion make any sense to you? Otherwise, could we find some docshell/history experts to help us figure this out?
Also, do we have any unit test for app redirects? If not, it may have been broken recently...

Comment 3

[[:fabrice] Fabrice Desré]

I can reproduce the failure with a test app here also, and will start bisecting. I'm gonna check with QA also since the facebook import relies on this and that was not flagged as broken recently.

Comment 4

[Jason Smith [:jsmith]]

Fabrice mentions this affects fb import, which means this isn't a devtools specific bug - it's a core bug.

Comment 5

[[:fabrice] Fabrice Desré]

I checked with tchung and on my own build, but FB import is not affected. Still investigating though.

Comment 6

[[:fabrice] Fabrice Desré]

And I can't reproduce either with another oauth api (dropbox).

Comment 7

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> I can reproduce the failure with a test app here also, and will start
> bisecting. I'm gonna check with QA also since the facebook import relies on
> this and that was not flagged as broken recently.

Can you attach the test app? I can get someone to do a regression window here if I can define clear STR with the test app to do the regression window against.

Comment 8

[[:fabrice] Fabrice Desré]

(In reply to Jason Smith [:jsmith] from comment #7)
> (In reply to Fabrice Desré [:fabrice] from comment #3)
> > I can reproduce the failure with a test app here also, and will start
> > bisecting. I'm gonna check with QA also since the facebook import relies on
> > this and that was not flagged as broken recently.
> 
> Can you attach the test app? I can get someone to do a regression window
> here if I can define clear STR with the test app to do the regression window
> against.

That was actually a network error on the service side. I can't repro for now, so I'll check with ochameau if we can bisect the test failure instead.

Comment 9

[Alexandre Poirot [:ochameau]]

I reclassified this bug and removed koi? as it looks like a test specific issue that isn't landed yet because of this assertion. So that this bug doesn't seem that serious at the end. The redirects features has been confirmed to work fine.

I'll try to dig into this assertion when I get some free cycle.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Is there still an issue here or can we close this out?

Comment 11

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02d8bd7e8e90

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Add test for redirects in webapps actor mochitest

Rebased, with a new try, to see if the assertion still happens.

Comment 13

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a9d145ea1f5

Comment 14

[Alexandre Poirot [:ochameau]]

There is no more assertion with this test, but it doesn't work anymore and I don't know why.
It would still be great to cover this feature with a test... especially since the test already somewhat exists.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

I'll leave it to you to mutate the bug accordingly in that respect, thanks for the update.

Comment 16

[Alexandre Poirot [:ochameau]]

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

Comment 17

[Alexandre Poirot [:ochameau]]

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

Comment 18

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4455b861da8b

Comment 19

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

Apps are gone now.