Closed Bug 1577819 Opened 3 months ago Closed 3 months ago

Fix a few issues with ChromeUtils::RequestProcInfo()

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

I was reading over this code, and I think there are a few issues I want to clean up.

  • mozilla::dom::ProcType is too similar of a name to mozilla::ProcType, so I'm going to rename it to mozilla::dom::WebIDLProcType.
  • ProcType gets static casted to the WebIDL ProcType, but right now they don't actually match. I'm not sure if the idea is that only ProcType values that correspond to the WebIDL proctype should be cast there, but it seems more direct to just require that they are the same. I update the WebIDL ProcType and add a static assert that they at least have the same number of cases, and add a comment to ProcType mentioning the WebIDL ProcType.
  • ChromeUtils::RequestProcInfo() tries to match a few things against processType. Right now, if any of those fail to match you end up with an uninitialized type. This causes intermittent failures with Fission. I've added an else that sets it to Unknown.
  • browser_test_procinfo.js doesn't do any checks of the |type| field. I added a few basic checks for it to the test, which makes this consistently fail with Fission, as expected.

I'm going to leave fixing this with Fission for a separate bug, as I think this is enough for now.

There is also a mozilla::ProcType, which makes things
confusing. dom::ProcType seems to be used only for passing values to
JS via WebIDL, so I put WebIDL in the name.

ProcType gets static casted to WebIDLProcType. I assume the intention
is that they are identical. At a minimum, there should be a static
assert that both enums have the same number of cases.

Right now, the type ends up being uninitialized if the type doesn't
match.

The test for procinfo doesn't check if the process type is
reasonable. With Fission, this test now asserts, so add a failure
annotation. I'll fix it up for Fission in another bug.

Attachment #9089450 - Attachment description: Bug 1577819, part 3 - If processType fails to match in RequestProcInfo, set it to unknown. → Bug 1577819, part 3 - Initialize process type in RequestProcInfo.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/01e8fd20bdc1
part 1 - Rename mozilla::dom::ProcType to mozilla::dom::WebIDLProcType. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/dd470e3dff60
part 2 - WebIDLProcType should match ProcType. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/a9ac86bbe8d0
part 3 - Initialize process type in RequestProcInfo. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/98059acdf5be
part 4 - Add some basic validity checks for the type. r=Ehsan
You need to log in before you can comment on or make changes to this bug.