Closed Bug 767978 Opened 9 years ago Closed 9 years ago

Create an shistory object for top-level b2g <iframe mozbrowser>.

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Attachments

(2 files)

Bug 757137 appears to have caused a gaia bug [1], in which apps don't have session histories.

bz suggested we could ensure that all <iframe mozbrowser>'s have a shistory.  That seems like the easiest thing to do here.

[1] https://github.com/mozilla-b2g/gaia/issues/1869#issuecomment-6520055
Attached patch Patch v1Splinter Review
Attachment #636308 - Flags: review?(bzbarsky)
Blocks: 757137
Assignee: nobody → justin.lebar+bug
Comment on attachment 636308 [details] [diff] [review]
Patch v1

How could GetSessionHistory possibly return non-null here?
Comment on attachment 636308 [details] [diff] [review]
Patch v1

Also, you need to fix the various places that assume that only same-type roots will have a non-null mSessionHistory (e.g. nsDocShell::RemoveFromSessionHistory, various consumers of GetRootSessionHistory, etc).
Attachment #636308 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky (:bz) from comment #2)
> Comment on attachment 636308 [details] [diff] [review]
> Patch v1
> 
> How could GetSessionHistory possibly return non-null here?

> Also, you need to fix the various places that assume that only same-type roots will have 
> a non-null mSessionHistory (e.g. nsDocShell::RemoveFromSessionHistory, various consumers 
> of GetRootSessionHistory, etc).

Ah, I didn't realize that's what mSessionHistory was.  Dang.

Let's just fix this in the caller, then.  I've been trying to avoid hacking this docshell shistory stuff; I'm afraid of falling into a rathole.  (e.g. mozbrowser.canGoBack is only correct OOP, atm.)
Attached patch Patch, v1Splinter Review
Attachment #636370 - Flags: review?(bzbarsky)
Summary: Always create an shistory object for <iframe mozbrowser>. → Create an shistory object for top-level b2g <iframe mozbrowser>.
So is the shell.js code creating a toplevel docshell?
(In reply to Boris Zbarsky (:bz) from comment #6)
> So is the shell.js code creating a toplevel docshell?

The only thing above this <iframe> is a <xul:window windowtype='navigator:browser'>.  I'm not sure if that answers your question...
Not quite.  Let's try again.

1)  Does this <iframe> have a typeContent docshell?
2)  If so, is its parent docshell, if any, typeChrome?
(gdb) p mItemType
$1 = 1             # typeContent
(gdb) p mParent->mItemType
$2 = 0             # typeChrome
Comment on attachment 636370 [details] [diff] [review]
Patch, v1

r=me
Attachment #636370 - Flags: review?(bzbarsky) → review+
For my information, what was the gotcha we were checking for here?
Just the same "docshell looks for the session history on the root same-type docshell" bit.
(In reply to Boris Zbarsky (:bz) from comment #13)
> Just the same "docshell looks for the session history on the root same-type
> docshell" bit.

Thanks.

Sicking realized that, if we don't fix docshell shistory for in-process apps (comment 4), in-process apps should simply not have shistory.  IOW, we should back this out once we're running OOP all the apps we intent to run OOP.  (The plan is to run everything except the browser app OOP, because if we want to run browser content OOP, we'd need nested content processes, bug 761935.)
Assignee: justin.lebar+bug → nobody
Component: Document Navigation → General
Product: Core → Boot2Gecko
QA Contact: docshell → general
Version: Trunk → unspecified
Assignee: nobody → justin.lebar+bug
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 768558
https://hg.mozilla.org/mozilla-central/rev/c94c8777d7eb
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.